-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
🐛 add exports in package.json #4048
base: main
Are you sure you want to change the base?
Conversation
I think more is needed... "types" is at the top and then usually "./package.json" is by itself outside of the "." block. |
@probablyup can you give more specific info? |
I just don't see why it's needed... why isn't the |
it's a sample project that has error when running test with vitest. |
@probablyup I cannot 100% explain why @probablyup would you accept a backport to 5.x aswell? As we currently would like to stay with our project on 5.x because of typing issues but running into a problem with the export maps aswell there. So IMHO this affects everyone that wants to use styled-components with node.js based SSR in an ES modules environment. To add here and quote two examples of favourite toolchains, also Webpack and Rollup are advocating using the |
Further digging into this it seems that the exports have been removed because of a problem with typings (#3800 (comment), hence #3961). Actually IMHO the root cause of the issue with the types could be a "misconfigured" .tsconfig file where the "old" node module resolution (prior to v16) is used and not the current one. -> microsoft/TypeScript#51862 (comment) This "new" resolution algorithm is available since Typescript 4.7. Maybe @DavidReinberger could jump in to confirm that using the new resolution algorithm solves his problem with the types. |
Honestly I feel very uncomfortable with the |
FYI I have done some extensive research on this topic during this week which I would like to summarize here. Main assumptions
Node.js and
|
Thank you very much for this, will review these materials today |
To complicate things, I think there needs to be separate entries in "exports" for the |
b44b7e5
to
bfa2e5b
Compare
I've published the code on this branch at |
"import": { | ||
"node": "./dist/styled-components.esm.js", | ||
"default": "./dist/styled-components.browser.esm.js" | ||
}, | ||
"require": { | ||
"node": "./dist/styled-components.cjs.js", | ||
"default": "./dist/styled-components.browser.cjs.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.
Those two share the same directory, they are meant to represent two different module formats (CJS vs ESM) and yet they are using the same extension (.js
). This isn't correct and will cause problems one way or another. At the very least you need to either put those in different directories with appropriate package.json
scopes or use different extensions for those files (with the current content of the package.json
file you should use .mjs
for your ESM modules)
@@ -10,10 +10,33 @@ | |||
"./dist/styled-components.esm.js": "./dist/styled-components.browser.esm.js", | |||
"./dist/styled-components.cjs.js": "./dist/styled-components.browser.cjs.js" | |||
}, | |||
"exports": { | |||
".": { | |||
"types": "./dist/index.d.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.
Using types
like this when you are shipping dual packages isn't exactly correct and validators like arethetypeswrong will (rightfully) complain about this. If your runtime is dual, your types should be dual as well.
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.
Yeah, I couldn't figure out how to emit mts/cts using rollup. Will need to dig into tsup's source and see how they did it
@@ -10,10 +10,33 @@ | |||
"./dist/styled-components.esm.js": "./dist/styled-components.browser.esm.js", | |||
"./dist/styled-components.cjs.js": "./dist/styled-components.browser.cjs.js" | |||
}, | |||
"exports": { |
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.
You are attempting to ship so-called dual package here, thus you are making yourself vulnerable to the dual package hazard. Since styled-components rely on React context, I think that you should try to avoid it as much as possible. Using .mjs
wrappers strategy would be a much better choice that would be safer for the users.
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.
@Andarist I find this very interesting - do you know of any blogpost / resource where the .mjs
wrappers strategy is explained it detail, also in regards to 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.
It's primarily described in node.js docs here but you can also find references to it in webpack and esbuild docs. We use this strategy in Emotion.
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.
ok, thanks. I read that, but got confused with regards to Typescript and the issues described above. I'll have a look on how Emotion handles this as reference for my own lib, thanks!
"node": "./dist/styled-components.esm.js", | ||
"default": "./dist/styled-components.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.
This order isn't exactly incorrect but I think it makes sense to think of node as the default and use browser as an override. Either way, you probably want to also include the worker
condition here that will point to the non-browser code to accommodate Next.js and some other tools when they bundle for the edge workers target.
i have some issue that using vitest.
It always import cjs version for styled-components, i want to import esm version.
add exports into package.json that can help it 🙏