-
Notifications
You must be signed in to change notification settings - Fork 316
Improve ESM package entrypoints #1597
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b9fbff2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -10,9 +10,11 @@ | |||
"./package.json": "./package.json", | |||
".": { | |||
"browser": { | |||
"import": "./dist/vanilla-extract-css-adapter.browser.esm.js", | |||
"module": "./dist/vanilla-extract-css-adapter.browser.esm.js", |
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.
Where package entrypoints were already specified, they were left alone, "import"
was merely added to ensure ESM gets picked up
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.
IMO "module"
should just be removed—it’s usually ignored—but I’d wait for a breaking version in the future to remove it. There’s no harm to keeping it around short-term
"import": "./dist/vanilla-extract-compiler.cjs.js", | ||
"default": "./dist/vanilla-extract-compiler.esm.js" | ||
}, | ||
"./*": "./*" |
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.
For net-new exports
, it prevents importing anything other than the default. "./*": "./*"
allows everything to be imported, which preserves backwards-compatible behavior for everyone (i.e. this only guides the default ESM import to the correct location, without disrupting anything else)
7dc7119
to
4a0ff4d
Compare
@drwpow Updating entrypoints has been on my TODO list for a while, but I never got around to doing it I didn't want the update to result in a breaking change (without code changes to go with it). Thanks for getting the ball rolling. The package exports are primarily handled by I'm honestly not too fussed about maintaining backwards compatibility as far as importing from |
Dunno if you’ve checked out unbuild, but it’s hands-down my favorite packager at the moment. The thing I love about it explicitly is it uses I’ll have to check out tsdown. I’m a big fan of how Rolldown is shaping up, but I know it still has a few papercuts. And I’ve had issues with tsup (around entrypoints, funny enough), so not sure if tsdown fixes them, but that’d be great if it did. Very excited about anything built on Rolldown in general
Yeah I might have missed something here in regards to autogenerating—if there’s deeper work here then feel free to close! This could just be used as a suggestion for later, that’s fine.
That’s good to know. For the most part, yeah, most folks won’t notice. The only thing I wasn’t sure about was the fact that some libraries seemed to have a |
I have. I do enjoy the package.json exports-first approach to managing entrypoints. I'll consider it when I get around to migrating off
I should've clarified. I think they were auto-generated a while ago, but have had some manual tweaks since then, but are mostly stock (I think). Happy with this PR doing some more manual tweaks, but ideally we'd move to a tool that's more hands-off.
I'm not that concerned about the
That would be great too. I've gotten into the habit of doing this in other libraries too, so it would be much appreciated. |
Changes
The packages’ entrypoints could be improved. Specifically:
module
is not a valid entrypoint (and has never been—it’s simply been “allowed” but never been officially a standard). It’s now-replaced withexports
"import"
, not"module"
(docs) (however, just to not break anything,"module"
was left alone)"./*": "./*"
was added to preserve backwards-compatible behavior. Whenever"exports"
is specified, all imports are forbidden by default, and this explicitly re-allows manual importing."main"
was also left alone to preserve backwards-compatible support for … well, everything should be listening toexports
now, but y’never knowAll this will result in free, improved backwards-compatible ESM support for all packages. This shouldn’t cause any breaking changes for anyone, only improve bundler compatibility today and for the future.
Note: I was originally starting on #1588, but got distracted when I noticed the package ESM entrypoints needed updating
Feedback