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

Webpack and multiple babel modules are production dependencies #643

Closed
tommoor opened this issue Jul 27, 2020 · 11 comments
Closed

Webpack and multiple babel modules are production dependencies #643

tommoor opened this issue Jul 27, 2020 · 11 comments
Assignees
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: webpack 📦 Issue is related to webpack bundler cat: performance 🚀 Issue is related to performance needs: complete repro 🖥️ Issue need to have complete repro provided

Comments

@tommoor
Copy link

tommoor commented Jul 27, 2020

Environment

  • Linaria version: 2.0.0-rc.0"
  • Bundler (+ version): Yarn 1.22.4
  • Node.js version: v12.16.1
  • OS: macOS

Description

Webpack and multiple babel modules are marked as production dependencies, inflating build sizes and production install times.

See package.json: https://github.com/callstack/linaria/blob/master/package.json#L125

=> Found "watchpack#fsevents@2.1.3"
info Reasons this module exists
   - "remirror#@remirror#core#linaria#webpack#watchpack#chokidar" depends on it
   - Hoisted from "@elasticprojects#abstract-core#remirror#@remirror#core#linaria#webpack#watchpack#chokidar#fsevents"
info Disk size without dependencies: "56KB"
info Disk size with unique dependencies: "56KB"
info Disk size with transitive dependencies: "56KB"
info Number of shared dependencies: 0
@tommoor tommoor added bug report 🦗 Issue is probably a bug, but it needs to be checked needs: complete repro 🖥️ Issue need to have complete repro provided needs: triage 🏷 Issue needs to be checked and prioritized labels Jul 27, 2020
@github-actions github-actions bot added bundler: webpack 📦 Issue is related to webpack bundler cat: performance 🚀 Issue is related to performance and removed needs: triage 🏷 Issue needs to be checked and prioritized labels Jul 27, 2020
@ifiokjr
Copy link

ifiokjr commented Jul 30, 2020

Just to give some context, I'm the maintainer of remirror and in a recent update I've switched from using emotion to linaria.

For the most part this has been great, but I need to also include linaria as a runtime dependency which means consumers of my library are also installing it and all of its dependencies like webpack, babel, acorn.

This has caused a few issues with dependency clashes etc. For example, the latest version of next.js is using webpack@5 and can't coexist with linaria.

As a library author my choices are:

  • drop the linaria dependency from remirror and roll an alternative (I'd really rather not since this library is so good!)
  • ask that you split the functionality into separate packages for the build tools and the runtime code. @linaria/core, @linaria/react for runtime and linaria for the build tools.
  • ask that you convert the build tool dependencies to peerDepedencies.
  • suck it up (who needs happy users anyway).

I'm not sure how to proceed, but maybe others consuming this tool as part of a library are facing similar issues.

@satya164
Copy link
Member

webpack should be a peer dep. can you send a PR for that?

regarding deps like babel, the only way is to split up the package to multiple parts such as webpack loader, babel plugin and the core API. which we plan to do in future.

@wereHamster
Copy link
Contributor

I don't think moving webpack to peer deps is a good solution. If you use linaria in a nextjs project, you'll have two webpack versions installed in your node_modules: 4.x because of linaria and 5.x because of nextjs. Also, when you move webpack to peer deps, you can't have two different packages in your project which require a different major version of webpack peer dep (say, linaria which requires 4.x and package Y which requires 5.x). Lastly, if I don't use webpack in my project (I use rollup), I'll be seeing a warning about missing peer deps, and while I know I can ignore it in my case, others may not and will install webpack even if they don't need to.

I'd much rather prefer a separate package for the webpack loader, that package can have its own dependencies that are tracked independently of linaria core.

@satya164
Copy link
Member

satya164 commented Jul 31, 2020

Also, when you move webpack to peer deps, you can't have two different packages in your project which require a different major version of webpack peer dep (say, linaria which requires 4.x and package Y which requires 5.x).

Require is a strong word. Peer dependency isn't really enforced so no reason that you need to have 2 different versions. Unless there's real API incompatibilities.

Anyway, separate package is a the long term solution and it'll take some time. Moving it to peer deps is something that can be done quickly. There's no reason not to do it and then do the separate package after it.

@Anber
Copy link
Collaborator

Anber commented Aug 10, 2020

Actually, the only reason why we need webpack here is LoaderContext type from it. We can probably import it as import type in order to drop it from the bundle.

@ifiokjr
Copy link

ifiokjr commented Aug 11, 2020

@Anber that would be useful, but the fact that webpack is a dependency at all means it creates many conflicts when being used within a webpack consuming application. For example, I'm currently holding back from upgrading next.js to 9.5 because of the clash in webpack versions with linaria.

Unless of course you mean use import type while also converting webpack to a peerDependency.

@Anber
Copy link
Collaborator

Anber commented Aug 12, 2020

@ifiokjr webpack is used only for import LoaderContext, which is mostly used for interaction with loader-utils, so I reimported LoaderContext from loader-utils and dropped webpack from dependencies. loader-utils itself doesn't need webpack and requires only @types/webpack.

@Anber
Copy link
Collaborator

Anber commented Aug 12, 2020

@babel/core will be also moved to peerDependencies.
Related PR

@Anber Anber self-assigned this Aug 12, 2020
@ifiokjr
Copy link

ifiokjr commented Aug 12, 2020

@Anber really excited about the progress 🙌

@Anber
Copy link
Collaborator

Anber commented Aug 14, 2020

Done!

@wereHamster
Copy link
Contributor

This is a breaking change relative to 2.0.0-rc.0, my builds now fail with «Cannot find module 'webpack'» (next build) or «Cannot find module '@babel/core'» (rollup).

I get a warning about @babel/core being a missing peer dependency (as per #654), but no warning about webpack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: webpack 📦 Issue is related to webpack bundler cat: performance 🚀 Issue is related to performance needs: complete repro 🖥️ Issue need to have complete repro provided
Projects
None yet
Development

No branches or pull requests

5 participants