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

Fixed importing in Node ESM #3029

Merged
merged 4 commits into from
May 4, 2023
Merged

Fixed importing in Node ESM #3029

merged 4 commits into from
May 4, 2023

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented May 3, 2023

fixes #2973
fixes #2927
fixes #2730
fixes #2582

@Andarist Andarist requested a review from emmatown May 3, 2023 08:51
@changeset-bot
Copy link

changeset-bot bot commented May 3, 2023

🦋 Changeset detected

Latest commit: 2e01219

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
@emotion/babel-plugin Patch
@emotion/babel-plugin-jsx-pragmatic Patch
@emotion/babel-preset-css-prop Patch
@emotion/cache Patch
@emotion/css Patch
@emotion/css-prettifier Patch
@emotion/eslint-plugin Patch
@emotion/hash Patch
@emotion/is-prop-valid Patch
@emotion/jest Patch
@emotion/memoize Patch
@emotion/native Patch
@emotion/primitives Patch
@emotion/primitives-core Patch
@emotion/react Patch
@emotion/serialize Patch
@emotion/server Patch
@emotion/sheet Patch
@emotion/styled Patch
@emotion/unitless Patch
@emotion/use-insertion-effect-with-fallbacks Patch
@emotion/utils Patch
@emotion/weak-memoize Patch

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 3, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2e01219:

Sandbox Source
Emotion Configuration
Emotion issue template Issue #2927

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care that this is kind of breaking in that you previously could have done the below thing?

import styled from '@emotion/styled'
styled.default(...)

Maybe it's fine since that would also be broken in many bundler environments so the reliable thing would have been to do styled.default || styled?

Though it wouldn't be that hard to do styled.default = styled and then nothing would break, idk wdyt?

@Andarist
Copy link
Member Author

Andarist commented May 3, 2023

Do we care that this is kind of breaking in that you previously could have done the below thing?

I don't think so. Yes, somebody could depend on this behavior - but we never considered it to be alright. Nothing like that is in our docs. I consider this PR to be a bug fix but, of course, the lines is quite blurry here

Though it wouldn't be that hard to do styled.default = styled and then nothing would break, idk wdyt?

It would work... but then it would likely complain at type-level about this stuff (which reminds... I probably need to test this PR with moduleResolution: bundler, I forgot that Emotion isn't authored in TypeScript so Preconstruct isn't responsible for emitting correct types here).

I'm rather against this though. There are probably not that many ESM early adopters out there that we should take this into a consideration here.

@emmatown
Copy link
Member

emmatown commented May 3, 2023

Preconstruct isn't responsible for emitting correct types here

Because there are .d.ts files next to the js entrypoints, Preconstruct is kinda responsible for it (though it just really copies things)

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming you checked it in moduleResolution: bundler & maybe nodenext as well, looks good

Comment on lines 1 to 2
export * from './macro.js'
export { _default as default } from './macro.default.js'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export * from './macro.js'
export { _default as default } from './macro.default.js'
export { default } from '@emotion/styled'
export * from '@emotion/styled'

+ delete the macro.default.d.ts + do this for the other packages

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, why is that? 🤔 it seems to me that TS would trip over this without macro.default.d.ts in certain configurations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're in a .mts and we're resolving @emotion/styled, we'll hit the import condition we have for "." and get the already correct default.

Copy link
Member Author

@Andarist Andarist May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, that makes sense 👍 i wouldn't use a self reference here myself (it makes sense to use it - i just wouldnt think of it myself) but it was already here and it makes sense that with it we can simplify this a little bit. (EDIT:// ah, the self reference in this particular file is part of your proposed change, nice)

"./macro": {
"types": {
"import": "./macro.d.mts",
"default": "./macro.d.ts"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"default": "./macro.d.ts"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, i wasn't sure if this would "fall through" to the other default... does it? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does (but that's correct behaviour, not like TS's bug)

Comment on lines +72 to +78
"./macro": {
"types": {
"import": "./macro.d.mts",
"default": "./macro.d.ts"
},
"default": "./macro.js"
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not a fan of array flavor... maybe, in here, this would actually be a little bit more obvious that the "fallthrough" is intended?

Suggested change
"./macro": {
"types": {
"import": "./macro.d.mts",
"default": "./macro.d.ts"
},
"default": "./macro.js"
}
"./macro": [
{
"types": {
"import": "./macro.d.mts"
}
},
"./macro.js"
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, just leave it how it is. (it looks like arethetypeswrong incorrectly flags this as a problem, will open an issue about that in a moment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants