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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 add exports in package.json #4048

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

馃悰 add exports in package.json #4048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1e7896
:bug: add exports in package.json
RoySung Jun 21, 2023
bfa2e5b
refactor(package): adjust exports for entrypoints
quantizor Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/styled-components/native/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"name": "styled-components/native",
"private": true,
"types": "./dist/native/index.d.ts",
"main": "./dist/styled-components.native.cjs.js",
"jsnext:main": "./dist/styled-components.native.esm.js",
"module": "./dist/styled-components.native.esm.js"
"types": "../dist/native/index.d.ts",
"main": "../dist/styled-components.native.cjs.js",
"jsnext:main": "../dist/styled-components.native.esm.js",
"module": "../dist/styled-components.native.esm.js"
}
25 changes: 24 additions & 1 deletion packages/styled-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Copy link
Contributor

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.

Copy link

@Perni1984 Perni1984 Aug 2, 2023 •
edited

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?

Copy link
Contributor

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.

Copy link

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!

".": {
"types": "./dist/index.d.ts",
Copy link
Contributor

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.

Copy link
Contributor

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

"import": {
"node": "./dist/styled-components.esm.js",
"default": "./dist/styled-components.browser.esm.js"
Comment on lines +17 to +18
Copy link
Contributor

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.

},
"require": {
"node": "./dist/styled-components.cjs.js",
"default": "./dist/styled-components.browser.cjs.js"
}
Comment on lines +16 to +23
Copy link
Contributor

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)

},
"./macro": {
"types": "./dist/index.d.ts",
"import": "./dist/styled-components-macro.esm.js",
"require": "./dist/styled-components-macro.cjs.js"
},
"./native": {
"types": "./dist/native/index.d.ts",
"import": "./dist/styled-components.native.esm.js",
"require": "./dist/styled-components.native.cjs.js"
}
},
"sideEffects": false,
"scripts": {
"generateErrors": "node scripts/generateErrorMap.js",
"prebuild": "rimraf dist && yarn generateErrors",
"prebuild": "rimraf dist native/dist && yarn generateErrors",
"build": "rollup -c",
"postbuild": "yarn size",
"pretest": "yarn generateErrors",
Expand Down
4 changes: 2 additions & 2 deletions packages/styled-components/rollup.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,10 @@ const nativeConfig = {
input: './src/native/index.ts',
output: [
getCJS({
file: 'native/dist/styled-components.native.cjs.js',
file: 'dist/styled-components.native.cjs.js',
}),
getESM({
file: 'native/dist/styled-components.native.esm.js',
file: 'dist/styled-components.native.esm.js',
}),
],
plugins: configBase.plugins.concat(minifierPlugin),
Expand Down