Build: Fix CRA repro generator and e2e test in PnP mode #17375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 4abf608. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
Locally I'm getting this: Which is different form the CI. |
|
facebook/create-react-app#9442 facebook/create-react-app#9501 yarn set version classiclooks like --use-npm does nothing for CRA, it should be using npm? Why would the above help? |
|
Seems like the failure happens before it's installed?! And then the install is halted half way in?
|
ndelangen
force-pushed
the
fix/e2e-cra
branch
from
869a9d1 to
d3305f8
Compare
|
@shilman review away |
|
oof: |
|
When I ran it locally, |
|
I added artifacts to CI to preserve the end state of the e2e dir, and it seems like it too has the dependiencies in place: |
And this in storybook/core: @shilman does core really need webpack5 as a dependency?!? |
|
Probably technically yes but I couldn't tell you why from memory (and am afk right now). Maybe @merceyz remembers. |
# Conflicts: # lib/core/package.json # yarn.lock
ndelangen
commented
Feb 1, 2022
| @@ -87,7 +87,7 @@ export async function baseGenerator( | |||
| const addonPackages = [...addons, '@storybook/addon-actions']; | |||
|
|
|||
| const yarn2Dependencies = | |||
| packageManager.type === 'yarn2' ? ['@storybook/addon-docs', '@mdx-js/react'] : []; | |||
| packageManager.type === 'yarn2' ? ['@storybook/addon-docs', '@mdx-js/react@1.x.x'] : []; | |||
There was a problem hiding this comment.
@shilman version lock it to v1
|
|
|
reminder to remove the saving of artifacts (or ZIP them first) |
|
@gaetanmaisse this line might be a problem: though it seems this code isn't used at all! and this one: storybook/lib/core-common/src/utils/es6Transpiler.ts Lines 6 to 38 in eb2fb1c What does something like this to a --pnp enabled repo?: |
From the tests I did locally, adding both these deps is fixing Storybook in a CRA app with Yarn PnP.
|
@ndelangen with the last commit I pushed it unblocks webpack build, I'm not sure to understand why tho'... As a workaround to see what's the status without this issue, I copied the project locally and added the following in the (I also linked all the Storybook packages to have all the fixed done in this PR: I would be glad to discuss this with you to define our next move! |
|
@gaetanmaisse that sounds like CRA itself has a problem with pnp then? |
|
@gaetanmaisse I've fixed that issue in facebook/create-react-app#11751 but it hasn't been merged yet. The fix is included in the canary releases of Yarn though if you want to use that.
It's an undeclared dependency which is a problem in general, not just with PnP. |
|
@shilman @gaetanmaisse merge and wait for CRA to fix upstream? |
shilman
reviewed
Feb 16, 2022
lib/core/package.json
Outdated
| @@ -45,6 +45,7 @@ | |||
| }, | |||
| "peerDependencies": { | |||
| "@storybook/builder-webpack5": "6.5.0-alpha.39", | |||
| "@storybook/manager-webpack5": "6.5.0-alpha.39", | |||
There was a problem hiding this comment.
Can we do this for all optional peer dependencies? If so, I think that could be an improvement
| @@ -86,6 +86,18 @@ | |||
| "@babel/core": { | |||
| "optional": true | |||
| }, | |||
| "@storybook/builder-webpack4": { | |||
There was a problem hiding this comment.
Because there is @storybook/core as dep and it has @storybook/builder-webpack4 as optional peer dep itself, so we need to add it to satisfy transitivity. Maël did a post about it some time ago: https://dev.to/arcanis/implicit-transitive-peer-dependencies-ed0
| "peerDependenciesMeta": { | ||
| "@storybook/angular": { | ||
| "optional": true | ||
| }, | ||
| "@storybook/builder-webpack4": { |
|
@ndelangen I will review the comments and do the needed changes later today ;) |
As of CRA 5 `--use-npm` option has been removed. To force usage of NPM the only hacky way I found is to override `npm_config_user_agent` env var. See related PR: facebook/create-react-app#11322 And changelog: https://github.com/facebook/create-react-app/releases/tag/v5.0.0
We will need to discuss the `import/no-extraneous-dependencies` rule config
gaetanmaisse
changed the title
> TL;DR: Vue 3 is now the new default version as of Monday, February 7, 2022! > https://blog.vuejs.org/posts/vue-3-as-the-new-default.html
1 task
|
Soooo, after the issues with CRA I had to deal with Vue and
I updated the repro to use fixed versions of Vue related packages but there is maybe more to do on Everything 🟢 on CircleCI side! 🎆 Good to merge @shilman @ndelangen ? |
2 tasks
shilman
added
patch:yes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One of the CI tasks keeps failing.
Part of the issue was related to CRA:
As of CRA 5
--use-npmoption has been removed.To force usage of NPM the only hacky way I found is to override
npm_config_user_agentenv var.See related PR: Use env var to detect yarn or npm as the package manager facebook/create-react-app#11322
And changelog: https://github.com/facebook/create-react-app/releases/tag/v5.0.0
CRA 5 was missing some dependencies, causing Yarn to throw in PnP mode. See fix: resolve dependency issues facebook/create-react-app#11751
And some other things to Vue 3:
notes: