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

@emotion/react pulls babel into production builds on node 16/npm 8 #2588

Closed
BGehrels opened this issue Dec 10, 2021 · 8 comments
Closed

@emotion/react pulls babel into production builds on node 16/npm 8 #2588

BGehrels opened this issue Dec 10, 2021 · 8 comments

Comments

@BGehrels
Copy link

Current behavior:
@emotion/react has an optional peer dependency on @babel/core.

Node 14 LTS included NPM 6, which issued warnings on missing peer dependencies. Node 16 recently became LTS and it now includes NPM 8, which automatically installs peer dependencies. This behaviour can only be circumvented by enabling a --legacy-peer-deps flag, which - judging by it's name - is deprecated by design.

So, from Node 16 on, a production dependency on @emotion/react will automatically pull @babel/core into your production build.
Why is this bad?

  • Bundle size explodes
  • @babel/core depends on other packages like caniuse-lite which is licensed unter CC-BY-4.0 which triggers probably unexpected legal attribution requirements when being shipped as part of a React App.

To reproduce:

  • Create a package.json with @emotion/react as a production dependency
  • Execute npm i --production in a node 14 environment
  • npm ls caniuse-lite -prod will show no entry
  • upgrade to node 16
  • Execute npm i --production
  • npm ls caniuse-lite -prod will show
    webroot@1.0.0 /home/node/app/webapp
    `-- @emotion/react@11.6.0
      `-- @babel/core@7.16.0
        `-- @babel/helper-compilation-targets@7.16.0
          `-- browserslist@4.17.6
            `-- caniuse-lite@1.0.30001279

Expected behavior:
Installing @emotion/react should not pull babel into your production build

Environment information:
@emotion/react@11.6.0
node v16.13.1
npm 8.1.2

@Andarist
Copy link
Member

Bundle size explodes

How is that so? Even if this gets installed it will only grow your node_modules size but not the production bundle size (unless u import it in your app code for whatever reason).

Node 16 recently became LTS and it now includes NPM 8, which automatically installs peer dependencies

I understand your frustration but I don't fully agree that this is an issue with Emotion. The package is clearly stated as optional peer dependency - they shouldn't be installing it in the first place. AFAIK other package managers have raised concerns that how npm auto-installs peer deps is harmful. I don't know the full context behind their words but I believe there is a strong reason why other package managers don't do this. So from my PoV, this really looks like a problem with npm itself and not with Emotion.

@BGehrels
Copy link
Author

Yep, sorry about the bundle size thing, you're right.

What i'm wondering right now is why this artifact declares babel as a peer dependency at all. I can totally see this for @emotion/babel-plugin that only works within a babel 7 environment, but @emotion/react?

While i share your opinion that the recent change in Node is not for the better and quite contra-intuitive when it comes to optional dependencies, i would still propose having a thought about some mitigation here: Node releases a new LTS every 1,5 years or so, so even if this behaviour will change, it will be around for quite some time and we as a community will somehow have to cope with it.

@Andarist
Copy link
Member

It's because we export a Babel macro here: https://github.com/emotion-js/emotion/blob/01cca604ca93c5fd3958c41045ecbcac72b65480/packages/react/macro.js . And with such package structure we have to declare optional peer dependency for Babel as without it this wouldn't work at all with strict package managers like pnpm or Yarn Berry

@NMinhNguyen
Copy link

These packages don't seem to be specifying @emotion/babel-plugin as a dependency though - isn't that a violation when using strict package managers?

@Andarist
Copy link
Member

You are right that this wasn't declared properly by @emotion/react - it was in other packages though. I've prepared a fix for this here:
#2609

@NMinhNguyen
Copy link

You are right that this wasn't declared properly by @emotion/react - it was in other packages though. I've prepared a fix for this here: #2609

Thanks! But I wonder if @emotion/babel-plugin should be an optional peer dependency instead? Or perhaps importing it via @emotion/react should be deprecated, and users should directly import macros from @emotion/babel-plugin instead. Obviously this would be a breaking change, so your fix as it stands now is still valid. I'm just more thinking about the long-term fix.

@Andarist
Copy link
Member

Thanks! But I wonder if @emotion/babel-plugin should be an optional peer dependency instead?

It kinda should but this was deliberately chosen as a solution for its simplicity (you don't have to manually install @emotion/babel-plugin to use a macro)

and users should directly import macros from @emotion/babel-plugin instead.

While this would be technically possible and correct - it would look kinda weird for consumers so this is probably not something that we'd like to do. In most situations, macros are part of the package (or are the package) - we just reuse the logic for both macros and the Babel plugin so we import this instead of reimplementing stuff or spreading the Babel-related implementation details across packages.

@srmagura
Copy link
Member

Closing in favor of #2660 since both issues are really about the same thing.

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

No branches or pull requests

4 participants