Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Niko/node new template #547

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Niko/node new template #547

wants to merge 39 commits into from

Conversation

nikomancy
Copy link
Contributor

@nikomancy nikomancy commented Dec 9, 2024

Adding variant of docs for Node.js following the new information architecture and doc structure as the client.js docs.

Most of this content is duplicate boilerplate between client and server js.

Dependencies

These docs are built on top of the Docusaurus 2.x -> 3.x major version update. They shouldn't be merged before that PR is merged.

nikomancy and others added 21 commits December 5, 2024 10:28
* add precomputed assignments documentation for JS client

* very nice!

* globally

* Update docs/sdks/client-sdks/javascript/precomputed-assignments.mdx

Co-authored-by: Sameeran Kunche <[email protected]>

---------

Co-authored-by: Sameeran Kunche <[email protected]>
* Update precomputed-assignments.mdx evaluation example

* Update docs/sdks/client-sdks/javascript/precomputed-assignments.mdx

Co-authored-by: Sameeran Kunche <[email protected]>

---------

Co-authored-by: Sameeran Kunche <[email protected]>
* add precomputed assignments documentation for JS client

* very nice!

* globally

* Update docs/sdks/client-sdks/javascript/precomputed-assignments.mdx

Co-authored-by: Sameeran Kunche <[email protected]>

---------

Co-authored-by: Sameeran Kunche <[email protected]>
Adding Node.js specific verbiage.
* Opportunistic fix of an issue with JS initialization.
* Updating with node specific details.
Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for eppo-data-docs failed.

Name Link
🔨 Latest commit f027738
🔍 Latest deploy log https://app.netlify.com/sites/eppo-data-docs/deploys/677f6e54fda1350008e5cbef

nikomancy and others added 6 commits December 13, 2024 12:11
* add precomputed assignments documentation for JS client

* very nice!

* globally

* Update docs/sdks/client-sdks/javascript/precomputed-assignments.mdx

Co-authored-by: Sameeran Kunche <[email protected]>

---------

Co-authored-by: Sameeran Kunche <[email protected]>
This reverts commit 756e664.

Accidentally committed dot net files to node branch. Undoing.

 Changes to be committed:
	deleted:    docs/sdks/server-sdks/dotnet/_category_.json
	deleted:    docs/sdks/server-sdks/dotnet/assignments.mdx
	deleted:    docs/sdks/server-sdks/dotnet/bandits.mdx
	deleted:    docs/sdks/server-sdks/dotnet/initialization.mdx
	deleted:    docs/sdks/server-sdks/dotnet/intro.mdx
	deleted:    docs/sdks/server-sdks/dotnet/quickstart.mdx
@aarsilv aarsilv self-assigned this Dec 18, 2024
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the improvements--these docs are really shaping up! I have some random comments here and there on the content. Regarding the code examples ,there are a couple of issues I spotted:

  1. The Node.js package name is @eppo/node-server-sdk
  2. Our init() method type requires an assignmentLogger to be defined--although I think we should adjust our types in the SDK itself.
  3. We are inconsistent with our string quoting (we probably want to conform to ")
  4. We are inconsistent with our use of TypeScript and Vanilla JS (we probably want to conform to TypeScript)

Also, there are quite a few places we link to the client JavaScript documentation, which could confuse people as they navigate. We'll want to make sure all links, language, and examples are backend-related

docs/data-management/metrics/simple-metric.md Outdated Show resolved Hide resolved
docs/reference/webhook.mdx Show resolved Hide resolved

```javascript
import * as EppoSdk from "@eppo/js-client-sdk";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this same change in Quickstart.

Also, unfortunately*, our TypeScript types require assignmentLogger to be defined:

import { init } from "@eppo/js-client-sdk";

// One-time initialization to put in application bootup
await init({
  apiKey: "SDK_KEY",
  assignmentLogger: { logAssignment: (assignmentEvent)=> console.log('Send to warehouse: ', assignmentEvent) },
});

*this is probably something we should consider changing for dev-friendliess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My two cents are that changing this in the code depends on whether there are practical situations where you would want to not log assignments. In the meantime, I've updated code samples where init gets invoked to include the placeholder assignmentLogger you put there.

docs/sdks/sdk-features/obfuscation.mdx Show resolved Hide resolved
docs/sdks/server-sdks/dotnet.mdx Show resolved Hide resolved
docs/sdks/server-sdks/node/quickstart.mdx Outdated Show resolved Hide resolved
docs/sdks/server-sdks/node/quickstart.mdx Outdated Show resolved Hide resolved
docs/sdks/server-sdks/node/quickstart.mdx Outdated Show resolved Hide resolved
docs/sdks/server-sdks/node/quickstart.mdx Show resolved Hide resolved
docs/sdks/server-sdks/node/quickstart.mdx Show resolved Hide resolved
* Removing old node.mdx docs page.

* Aligned on using double quotes in code samples.

* Set all JS code samples to use TS syntax highlighting.

* Updated Bandit docs to have better attributes.

* Fixing the JS intro being for node and the node intro being for JS.
* Updating netlify redirects to force redirect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants