react-cosmos / react-cosmos

Sandbox for developing and testing UI components in isolation
https://reactcosmos.org
MIT License
8.36k stars 350 forks source link

babel-loader incompatible with project webpack #407

Closed NiGhTTraX closed 7 years ago

NiGhTTraX commented 7 years ago

What's up?

react-cosmos-webpack depends on babel-loader which peer depends on webpack. Since react-cosmos-webpack doesn't have a dependency on webpack you can run into cases where the project using it will have a webpack version that is incompatible with the babel-loader version.

Mkay, tell me more...

npm WARN babel-loader@6.4.1 requires a peer of webpack@1 || 2 || ^2.1.0-beta || ^2.2.0-rc but none was installed.

➜  npm ls babel-loader
├── babel-loader@7.1.1 
└─┬ react-cosmos-webpack@2.0.0-beta.20
  └── babel-loader@6.4.1 

➜   npm ls webpack
└── webpack@3.5.4 
ovidiuch commented 7 years ago

@NiGhTTraX any ideas?

Dropping webpack 1.x is inevitable, but don't think it addresses the larger issue.

NiGhTTraX commented 7 years ago

Maybe peer depend on babel-loader? I know that at one point the idea was to encapsulate as much as possible into the package, to make usage easier, but I always felt the build chain should be provided by the host project.

The webpack config is optional so that in turn makes every build related dep optional as well. Babel is optional on latest Node. Seems to me building the components is the host's responsibility.

ovidiuch commented 7 years ago

@NiGhTTraX this happens when you rely on Cosmos' default webpack config, right?

Solution #1:

We prioritize for people with latest version of webpack/Babel and upgrade the babel-loader version, so that only people with webpack 1 will have this problem. For them, the solution is to use a custom webpack config and point to an older version of babel-loaded they installed locally.

Solution #2

Remove babel-loader from deps (as you say), but check if user has it installed in default-webpack-config and only add it to loaders if they do.

Wdyt?

NiGhTTraX commented 7 years ago

@skidding you're going to get the warning whether you use the default webpack config or not.

Maybe I phrased the issue wrong, this is just an npm warning, it doesn't break anything.

It's just a matter of the cosmos package forcing the download of optional dependencies. You could use cosmos with Traceur and never want to download babel-loader. Same goes for the style loaders, maybe you never import CSS files in the components. I guess it's not the end of the world if you download some things you're never going to use, but with Babel you'll actually get a warning. One you can ignore but a warning nonetheless.

ovidiuch commented 7 years ago

I see what you mean. I'm thinking of removing all webpack loaders from react-cosmos-webpack deps. The only one I'm not sure about is json-loader, which is used for reading .json fixtures. Come to think of it, that could also be left for users to configure if they want it...

NiGhTTraX commented 7 years ago

Sounds like a plan. Those loaders are only there to support default-webpack-config, right? My assumption is that while the config might be useful for very basic projects, most have a more complex config that they need to pass to cosmos. I think it would also reduce confusion for projects that use the default config and then add something to their project config which then makes cosmos stop working.

ovidiuch commented 7 years ago

Sounds like a plan. Those loaders are only there to support default-webpack-config, right?

Yes

My assumption is that while the config might be useful for very basic projects, most have a more complex config that they need to pass to cosmos.

Probably true

I think it would also reduce confusion for projects that use the default config and then add something to their project config which then makes cosmos stop working.

It would definitely be an improvement. My plan is to make the default config a smart config. I.e. it starts as a bare webpack config, and then does stuff like look into the user's node_modules and if babel-loader is found, it creates a modules.loaders entry with it (same for a bunch of popular loaders like css-loader or url-loader). This is possible because the config file runs on the server–before we even start the dev server–so we have access to the file system.

NiGhTTraX commented 7 years ago

My plan is to make the default config a smart config

That would be the best of both worlds. Wouldn't be hard either, just

try {
  require('babel-loader');
  config.module.loaders.push(...);
} catch (e) { }
ovidiuch commented 7 years ago

Yeah. resolveFrom.silent, makes it even easier. Besides not having to try/catch ourselves, we can set the resolve base to something like dirname(cosmosConfigPath), which work great in the monorepo or when linking a Cosmos-dependent codebase to a local clone of Cosmos (useful when hacking Cosmos) – thus ensuring that we look for the loaders in the user's node_modules and not in those of the remote Cosmos copy.

Edit: import-from is even better.

ovidiuch commented 7 years ago

aaaaaaaaand done. Please review.

The diff is a bit scary so here's a timeline of what happened:

It's not that much code if you only look at the source code. @NiGhTTraX thanks!