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

chore(openapi): build script to setup ESM/CJS module exports + various fixes #10

Closed
wants to merge 4 commits into from

Conversation

gfortaine
Copy link

@gfortaine gfortaine commented Jan 4, 2023

1 PR to rule them all 😉 cc @schnerd @shun-shobon @ericlewis :

° Changelog

@gfortaine gfortaine changed the title chore(esm): enable ESM chore(node): enable ESM Jan 4, 2023
@gfortaine
Copy link
Author

Related openai/openai-node#45

@gfortaine gfortaine changed the title chore(node): enable ESM chore(node): enable ESM support Jan 4, 2023
@gfortaine gfortaine changed the title chore(node): enable ESM support chore(openapi): use typescript generator w/ fetch Jan 17, 2023
@ctjlewis
Copy link

ctjlewis commented Jan 19, 2023

Isomorphic __dirname is tricky but can be done, you have to make guarantees about the file structure but it can be achieved with a polyfill and added as a banner to Esbuild. See: https://github.com/tsmodule/tsmodule/blob/master/src/specification/shim.ts

Nice work on this also.

Copy link
Collaborator

@schnerd schnerd left a comment

Choose a reason for hiding this comment

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

Really liking this change overall. Left a few comments and still need to do some testing on the final package before proceeding, but overall this is looking promising.

config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
"prepublishOnly": "npm run prepare"
},
"dependencies": {
"@fortaine/fetch": "^6.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this different than @vercel/fetch? Would prefer not to use this fork if we can avoid it.

Copy link
Author

Choose a reason for hiding this comment

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

@vercel/fetch doesn't work with nodejs native fetch at the time of writing. A PR has been made : vercel/fetch#76

@schnerd
Copy link
Collaborator

schnerd commented Jan 22, 2023

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

@gfortaine
Copy link
Author

gfortaine commented Jan 22, 2023

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

@gfortaine
Copy link
Author

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

By the way, we tried with typescript-fetch in the first place. However, it seems that there is a nasty bug when using oneOf

@gfortaine gfortaine changed the title chore(openapi): use typescript generator w/ fetch chore(openapi): build script to setup ESM/CJS module exports + various fixes Mar 5, 2023
• chore(upgrade): get the version from the package.json w/o using require (prebuild script)
@logankilpatrick
Copy link
Contributor

Now that we have the updated NodeSDK, I am going to close this PR. Please let me know if there's further discussion needed here and thanks for diving in on this originally!

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