Nothing Special   »   [go: up one dir, main page]

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

🐛 add exports in package.json #4048

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

Conversation

RoySung
Copy link
@RoySung RoySung commented Jun 21, 2023

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 🙏

@quantizor
Copy link
Contributor

I think more is needed... "types" is at the top and then usually "./package.json" is by itself outside of the "." block.

@RoySung
Copy link
Author
RoySung commented Jun 25, 2023

@probablyup can you give more specific info?

@quantizor
Copy link
Contributor

I just don't see why it's needed... why isn't the module package.json field being respected? It's a common bundler pattern.

@RoySung
Copy link
Author
RoySung commented Jun 26, 2023

it's a sample project that has error when running test with vitest.
if i change version to 6.0.0-beta.12, it is working.
the version that has exports.

https://github.com/RoySung/vite-preact-playground

@Perni1984
Copy link
Perni1984 commented Jul 19, 2023

@probablyup I cannot 100% explain why module is not respected, but at least in the node.js world it was "just" a proposal (@see https://stackoverflow.com/questions/42708484/what-is-the-module-package-json-field-for). Node.js settled for the package.json exports field starting with node v16 and thus it's important to have proper export maps defined when you want to e.g. use styled components together with Next.js and SSR.

@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 exports field in package.json:

@Perni1984
Copy link
Perni1984 commented Jul 20, 2023

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)
https://www.typescriptlang.org/tsconfig#node16nodenext-nightly-builds

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.

@quantizor
Copy link
Contributor

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) https://www.typescriptlang.org/tsconfig#node16nodenext-nightly-builds

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 exports object... it seems to be very easy to screw up and it's like impossible to test the plurality of project configs out there to make this safe to add :/

@Perni1984
Copy link

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 module field

I can confirm that the module field is deliberately not supported by node.js (@see https://nodejs.medium.com/announcing-a-new-experimental-modules-1be8d2d6c2ff)

"While we are aware that the community has embraced the “module” field, it is unlikely that Node.js will adopt that field as many of the packages published using “module” include ES module JavaScript that may not evaluate in Node.js (because extensions are left off of filenames, or the code includes require statements, etc.)"

The module field in package.json was a proposal brought up by bundlers (for more context: originally from Rollup in 2017 - before there was a proposal for a jsnext:main field in 2015 which was superseded by the module field). So in the "bundler-world" the module field just works fine, but already the related github discussions for module and jsnext:main fields point out that there are some concerns regarding the ambiguity/flexibility of module and jsnext:main fields for different environments.

Instead Node choose to go forward with the exports field in package.json from >= v.16.x to provide

a modern alternative to "main" allowing multiple entry points to be defined, conditional entry resolution support between environments, and preventing any other entry points besides those defined in "exports". This encapsulation allows module authors to clearly define the public interface for their package.

For dual packages (CJS/ESM) like styled-components the exports field is the only way to support ES modules in a node.js environment (@see https://nodejs.org/docs/latest-v16.x/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards - both approaches use the exports field).

Bundlers and the exports field

As stated above all the popular bundlers (while also supporting the module field) also are advocating for the exports field for supporting different environments:

Typescript and exports field

Typescript historically had two module resolution algorithms called classic and node, whereas node was renamed to node10 half a year ago (microsoft/TypeScript#51901). As styled components is stating node16 as it's minimum requirement according to the Typescript Maintainers the moduleResolution field in .tsconfig should also switch to node16 consequently, which enables the exports field for types aswell. Moreover a detailed example of how to structure the package.json with exports field in a "most compatible" way is outlined in the typescript handbook:

{
    "name": "my-package",
    "type": "module",
    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": {
                // Where TypeScript will look.
                "types": "./types/esm/index.d.ts",
                // Where Node.js will look.
                "default": "./esm/index.js"
            },
            // Entry-point for `require("my-package")` in CJS
            "require": {
                // Where TypeScript will look.
                "types": "./types/commonjs/index.d.cts",
                // Where Node.js will look.
                "default": "./commonjs/index.cjs"
            },
        }
    },
    // Fall-back for older versions of TypeScript
    "types": "./types/index.d.ts",
    // CJS fall-back for older versions of Node.js
    "main": "./commonjs/index.cjs"
}

What is still unclear to me whether you need to set the type field to module in typescript to get exports field support. If so probably more needs to be changed, "type": "module" means .js files will be interpreted as ES Modules and the file ending .cjs needs to be used consequently.

Moreover this PR only adds one entry for types whereas Typescript maintainers recommend to have separate type declaration files for ESM and CommonJS even if they look the same.

Conclusion

In conclusion to have the broadest and most future proof support having an exports field in package.json is vital for styled components, and I don't see any way around it, if you want to support ES modules.

As I would assume styled components wants to support ES modules and CommonJS in a hybrid approach, @probablyup how can we support you in going forward?

@quantizor
Copy link
Contributor

As I would assume styled components wants to support ES modules and CommonJS in a hybrid approach, @probablyup how can we support you in going forward?

Thank you very much for this, will review these materials today

@quantizor quantizor self-assigned this Aug 1, 2023
@quantizor
Copy link
Contributor

To complicate things, I think there needs to be separate entries in "exports" for the styled-components/macro and styled-components/native entrypoints. I'm preparing a commit to add those to this PR. We'll need to ship this on the test tag and probably tweak it before releasing it to the masses.

@quantizor
Copy link
Contributor

I've published the code on this branch at styled-components@test for verification

Comment on lines +16 to +23
"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"
}
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)

@@ -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",
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

@@ -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

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.

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!

Comment on lines +17 to +18
"node": "./dist/styled-components.esm.js",
"default": "./dist/styled-components.browser.esm.js"
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.

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