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

lower babel-loader required version everywhere #14811

Merged
merged 4 commits into from Aug 2, 2021

Conversation

osdiab
Copy link
Contributor

@osdiab osdiab commented May 5, 2021

Issue: #5183

What I did

I reduced the version of babel-loader required by all the storybook subpackages to ^8.0.0 instead of ^8.2.2 so that modern versions of react-scripts can resolve the version it wants

How to test

  • Is this testable with Jest or Chromatic screenshots? Irrelevant
  • Does this need a new example in the kitchen sink apps? Nope
  • Does this need an update to the documentation? Nope

One thing that we should test is the missing dependency I saw here: #5183 (comment)

Not sure why the particular dependency was missing but i can't find mention of babel-plugin-polyfill-corejs2 in the storybook codebase, not really sure what was going on with that; maybe it was an addon?

@nx-cloud
Copy link

nx-cloud bot commented May 5, 2021

Nx Cloud Report

CI ran the following commands for commit a84c879. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@merceyz
Copy link
Contributor

merceyz commented May 5, 2021

This wont fix anything, babel-loader will be resolved to the latest version either way.

There is no issue on Storybooks' end, it's an issue with how create-react-app does their preflight check. Their preflight check assumes all their dependencies will be hoisted to the root which is not guaranteed to happen and is therefor a flawed check. They do get the expected version at runtime but their check is broken.

Ref #4764 (comment)

@osdiab
Copy link
Contributor Author

osdiab commented May 5, 2021

@merceyz i don't think that's true. If you install react-scripts, which specifies exactly Babel-loader 8.1.0 in its package.json, then yarn/NPM should resolve a mutually compatible version for both requirements I believe, unless I'm mistaken.

@ndelangen
Copy link
Member

The issue we're addressing here, is that storybook's babel dependencies were too 'latest' compared to CRA's, lowering them means we allow for more versions-ranges, which means we allow the resolve to the same version as CRA.

@osdiab
Copy link
Contributor Author

osdiab commented May 6, 2021

Looks like I was mistaken, when I tried it on a repo it did end up installing multiple versions and CRA complaining. But that said at least it's possible for me to modify my yarn.lock to install the version I desire instead of relying on breaking resolution for babel-loader for all subpackages of my company's monorepo by setting a Yarn resolutions entry override in the root package.json. still ugly but better than not being able to specify a valid version at all. and i wonder if based on the order of installing it people might not run into the issue at all.

@osdiab
Copy link
Contributor Author

osdiab commented May 6, 2021

and to clarify, by too "latest" i mean that the current version of react-scripts demands 8.1.1 but storybook specifies ^8.2.2 so they're mutually exclusive of one another (exact numbers might be off I'm just working off memory lol)

@ndelangen
Copy link
Member

We should try to keep our version ranges in package.json files of public packages, as low a possible, and leave it up to users to install the latest.

This will ensure we stay compatible with other tools as much as possible, and do not burden users with forcing them to upgrade other tools when they are not ready yet.

With "as low as possible" I mean:

- "babel-loader": "8.2.9",
+ "babel-loader": "8.0.0",

Unless we actually depend on something that was introduced in "8.2.x" of course. But I think generally we're not.

@goldo
Copy link

goldo commented May 31, 2021

Also interested in this PR ! 🙏 (react-script/babel-loader conflict)

@shilman
Copy link
Member

shilman commented May 31, 2021

A little bird told me CRA is going to drop this constraint in their next release

@osdiab
Copy link
Contributor Author

osdiab commented Jun 1, 2021

@shilman either way might still be worth doing this (and may make sense for other deps too) just to minimize conflicting versions on install, that definitely makes it less urgent if it materializes though!

@merceyz
Copy link
Contributor

merceyz commented Jun 1, 2021

facebook/create-react-app#10123 (comment) seems to indicate the specified version is what it is for a reason

@ndelangen ndelangen self-assigned this Jun 17, 2021
# Conflicts:
#	addons/graphql/package.json
#	addons/storyshots/storyshots-core/package.json
#	examples/riot-kitchen-sink/package.json
#	lib/core-server/package.json
#	yarn.lock
@shilman shilman added this to the 6.4 PRs milestone Jul 22, 2021
@shilman shilman added cra Prioritize create-react-app compatibility dependencies labels Jul 26, 2021
@shilman
Copy link
Member

shilman commented Aug 2, 2021

@ndelangen ping!

# Conflicts:
#	addons/storyshots/storyshots-core/package.json
#	examples/ember-cli/package.json
#	yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cra Prioritize create-react-app compatibility dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants