-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix(javascript): use exports
field to pick correct bundle
#947
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
exports
field to pick correct bundleexports
field to pick correct bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Small question :D
556aa4c
to
bf39894
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good after this fix!
"default": "./dist/{{packageName}}.umd.js" | ||
} | ||
}, | ||
"./src/*": "./src/*.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should rather be one of those:
{
"./src/*": "./src/*",
"./src/*.ts": "./src/*.ts",
}
Otherwise if there's another extension in the folder, it would incorrectly get imported as TypeScript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those clients all follow the same pattern with a single API file in their src
folder, we could be more precise on the name instead of using globs but it shouldn't change for now at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but it's less error prone and reduce the public API if we went for "./src/*.ts": "./src/*.ts"
. It would get tricky to debug if you encounter this bug. So we better address it now imo.
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/PRED-530 https://algolia.atlassian.net/browse/APIC-625
Slack thread: https://algolia.slack.com/archives/C0341QDM3EG/p1660811425419539
This updates the clients exports to use the
exports
field.There were issues in how clients were exported because the
module
assumed that it would get executed in a Node environment. This threw in multiple scenarios: Vite, Rollup, standard<script type="module">
, etc.We now rely on
exports
with two branches:"node"
to load the Node ESM and"default"
to load the browser ESM.Using
"browser"
instead of"default"
also fails with Rollup and@rollup/plugin-node-resolve
:For consistency, and to follow the Node recommendation, I also updated the
algoliasearch
browser export to use"default"
.Changes included:
exports
with"node"
and"default"
for all API clients"default"
instead of"browser"
foralgoliasearch
🧪 Test
No tests added, I believe the CI will do the work?