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

Prepare for code generation by moving generated code to separate files #15

Merged
merged 12 commits into from
Apr 3, 2024

Conversation

sam-1pass
Copy link
Contributor

@sam-1pass sam-1pass commented Mar 13, 2024

Move all constant code to client_util.ts and leave only generated code in client.ts in preparation for automating code generation.

@sam-1pass sam-1pass requested a review from hculea March 13, 2024 13:18
@sam-1pass sam-1pass self-assigned this Mar 13, 2024
@sam-1pass sam-1pass requested a review from AndyTitu March 14, 2024 15:34
@Marton6
Copy link
Member

Marton6 commented Mar 27, 2024

This looks good so far! Can you resolve the merge conflicts on it and give the MR a more descriptive title and description, @sam-1pass?

The description could mention why we're doing this: in preparation for automating code generation.

@sam-1pass sam-1pass changed the title Sam/code gen Separating static and generated files Mar 27, 2024
@Marton6 Marton6 changed the title Separating static and generated files Prepare for code generation by separating static and generated files Mar 28, 2024
@Marton6 Marton6 changed the title Prepare for code generation by separating static and generated files Prepare for code generation by moving generated code to separate files Mar 28, 2024
@Marton6
Copy link
Member

Marton6 commented Mar 28, 2024

@sam-1pass I improved the title a bit more to also express the intent behind the change. This makes it easier to understand why we made this change when we read the PR title

@@ -1,47 +1,5 @@
import * as os from "os";
Copy link
Member

Choose a reason for hiding this comment

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

In package.json we define the package's exports like so:

"exports": {
    ".": {
      "types": "./dist/client.d.ts",
      "default": "./dist/client.js"
    }
  },

This tells npm to look into client.js (which is compiled from client.ts) and make all exported types and functions accessible from outside the package.

By moving things out of this file, we're making them inaccessible to the SDK users. You can see that running the examples no longer works:

npm run commonjs-example

fails.

I think we should change where we export types/functions from so that all the types that used to be accessible to the SDK users will still be accessible after this MR is merged.

@AndyTitu, you know way more about packaging node projects. Is my assessment correct? How do you think we can best achieve this?

Also, could we write a unit test that would catch errors like this? (e.g. could we run the examples in the pipeline?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right, this refactor as it is right now would break the JS SDK. The exports.default acts as the entry point of the npm module. All that's exported in the exports.default file will be the default exports of the entire module, whereas exports.types marks the exported TypeScript types.

There are a few ways to fix this:

  1. Move the sdk.createClient function back into the client.ts file, since that's the only additional thing we would need to export to developers, besides the Client struct. (exporting the client struct will give users access to all the contained apis: secrets, items etc)

  2. Make client-util.ts the package entrypoint. In this case, I would like another name for this file, as it's not just an util holder but rather the entry point to the whole module. I'm thinking sdk.ts?

Copy link
Member

@hculea hculea Mar 28, 2024

Choose a reason for hiding this comment

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

Thanks for these options laying out! I was also thinking about solution 2, to ensure separation between generated code and the rest.

I am fine with swapping the name to sdk.ts 👍 - @sam-1pass can you have a look at that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into option 2 now 👍

Copy link
Member

Choose a reason for hiding this comment

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

@Marton6 do Sam's latest changes address this concern?

Copy link
Member

@Marton6 Marton6 Apr 2, 2024

Choose a reason for hiding this comment

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

Well does the SDK export the types and functions the user needs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested the new import experience for the latest code version:

Screenshot 2024-04-03 at 13 32 13

imo, this is everything the user needs access to for now: the Client, the createClient function and the util constants. These same components are exported for both typescript and javascript projects.

Copy link
Member

@Marton6 Marton6 Apr 3, 2024

Choose a reason for hiding this comment

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

This and the resolve function on the client.

I agree. If there was more stuff exported before and this is a breaking change, we should at least post a message about it in the developer workspace slack channel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that as well. But that's accessible via the client.secrets object as opposed to being a top level import

client/package.json Outdated Show resolved Hide resolved
AndyTitu added 2 commits April 3, 2024 13:29
… required functionality. Refactor logic in the expected files
Copy link
Member

@Marton6 Marton6 left a comment

Choose a reason for hiding this comment

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

Looks good to me but I haven't functionally reviewed it. I think @AndyTitu has tested it manually
.

Copy link
Member

@hculea hculea left a comment

Choose a reason for hiding this comment

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

I've confirmed that Andi functionally tested this. Should be good to go!

@sam-1pass sam-1pass merged commit 801c57e into main Apr 3, 2024
2 checks passed
@sam-1pass sam-1pass deleted the sam/code-gen branch April 3, 2024 13:22
@sam-1pass sam-1pass mentioned this pull request Apr 3, 2024
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