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.

Sign up for GitHub

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

ci: fix PnP e2e test #13059

Closed
wants to merge 2 commits into from
Closed

ci: fix PnP e2e test #13059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71f809d
test: fix e2e PnP test
merceyz Nov 9, 2020
8d4938b
fix: add react as a dependency for the vue test
merceyz Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scripts/run-e2e-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ export const sfcVue: Parameters = {
generator: fromDeps('vue', 'vue-loader', 'vue-template-compiler'),
additionalDeps: [
'webpack@webpack-4',
'react',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is what is required I'm assuming you don't actually want Vue users to have to install React. Perhaps Storybook should set an alias for react to make sure it's resolved from the core?

@gaetanmaisse seems to have "regressed" in #12972

Copy link
Member

@gaetanmaisse gaetanmaisse Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~It's weird as in the stated PR I added react to the deps of @storybook/vue: https://github.com/storybookjs/storybook/pull/12972/files#diff-3133f51bc714c0aa1f09aa806d0b57b4727c48342f4c570ffe531942fa17e074R42

So we shouldn't need to add react as a direct dep to make everything work together 🤔 ~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm pb comes from @storybook/addon-docs and not @storybook/vue:

ERR! Module not found: Error: @storybook/addon-docs tried to access react (a peer dependency) but it isn't provided by your application; this makes the require call ambiguous and unsound.
ERR!
ERR! Required package: react (via "react")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are back to the issue we discussed a week ago with other SB foxes: #12972 (comment)

@merceyz As a Yarn 2 master, do you have any idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Storybook should set an alias for react to make sure it's resolved from the core?

Are you speaking of a webpack alias? Cause I guess we already have one to be sure to use a single version of react, @ndelangen can confirm. However, it doesn't solve our missing (peer)dependency issues 😞

Copy link
Contributor Author

@merceyz merceyz Nov 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have any idea?

Either dependency injection or have @storybook/vue export react so that the vue part of the docs would import it from there but that only works if nothing else dependson react. The latter would give better treeshaking.

Are you speaking of a webpack alias?

Yes

However, it doesn't solve our missing (peer)dependency issues disappointed

That is true, if none of the dependencies the docs has needs react, importing react from @storybook/vue seems to be the best option

// TODO: remove when https://github.com/storybookjs/storybook/issues/11255 is solved
'core-js',
],
Expand Down
9 changes: 5 additions & 4 deletions scripts/run-e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,18 @@ const cleanDirectory = async ({ cwd }: Options): Promise<void> => {
const configureYarn2 = async ({ cwd }: Options) => {
const command = [
`yarn set version berry`,
// TODO: Remove on the next release of Yarn
`yarn config set yarnPath "${__dirname.replace(/\\/g, '/')}/yarn.js"`,
// ⚠️ Need to set registry because Yarn 2 is not using the conf of Yarn 1
`yarn config set npmScopes --json '{ "storybook": { "npmRegistryServer": "http://localhost:6000/" } }'`,
// Some required magic to be able to fetch deps from local registry
`yarn config set unsafeHttpWhitelist --json '["localhost"]'`,
// Disable fallback mode to make sure everything is required correctly
`yarn config set pnpFallbackMode none`,
// Add package extensions
// https://github.com/casesandberg/reactcss/pull/153
`yarn config set "packageExtensions.reactcss@*.peerDependencies.react" "*"`,
// https://github.com/casesandberg/react-color/pull/746
`yarn config set "packageExtensions.react-color@*.peerDependencies.react" "*"`,
// https://github.com/facebook/create-react-app/pull/9872
`yarn config set "packageExtensions.react-scripts@*.peerDependencies.react" "*"`,
`yarn config set "packageExtensions.react-scripts@*.dependencies.@pmmmwh/react-refresh-webpack-plugin" "*"`,
].join(' && ');
logger.info(`🎛 Configuring Yarn 2`);
logger.debug(command);
Expand Down
55 changes: 55 additions & 0 deletions scripts/yarn.js
View file
Open in desktop

Large diffs are not rendered by default.