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

Jump to bottom

Core: Add Yarn 2 compatibility #9667

Merged
merged 5 commits into from
Feb 4, 2020
Merged

Core: Add Yarn 2 compatibility #9667

merged 5 commits into from
Feb 4, 2020

Conversation

gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Jan 28, 2020 •
edited
Loading

Issue:
Part of #9527

🏗 Some changes will also be needed on CRA preset to make it compatible.

What I did

  • Move Webpack default outputDir outside @storybook/core package
  • Improve management of cache directory
  • Update Webpack configuration to handle PnP correctly
  • Add missing or fix implicit transitive peerDependencies

How to test

  "resolutions": {
    "@storybook/core": "portal:/PATH_TO_SB/lib/core"
  }
  • yarn install
  • yarn storybook and 🎉🎉

--
Does this need a new example in the kitchen sink apps?
--> I think so, but for now, I didn't find a way to use Yarn v2 on some project of our Yarn v1 workspace.

@shilman
Copy link
Member

shilman commented Jan 29, 2020 •
edited
Loading

Love this @gaetanmaisse !!!

Can you reuse an existing example?

@gaetanmaisse
Copy link
Member Author

gaetanmaisse commented Jan 29, 2020 •
edited
Loading

@shilman maybe cra-kitchen-sink or html-kitchen-sink because they both have a simple configuration with no presets. What's your point of view?

I still need to do a few more tests before setting this PR as ready but I think there will not be many more code changes on @storybook/core side 😉. Also, I started to look at the presets side (as CRA TS is not working for now):

  • ✅ @storybook/preset-typescript works without any change.
  • 🏗 @storybook/preset-create-react-app will need some updates. It's currently resolving react-script in node_modules/.bin/ directory (see here), so I need to add one more check for SB PnP user and access webpack config file directly in the .zip file with PnP API.

@shilman
Copy link
Member

shilman commented Jan 30, 2020

Yeah I’d say do both! CRA because it’s kind of “special” and one of the others to have a “normal” example. Would be nice to have one that tests typescript support too. If this is the future of yarn, doing a few should be fine. As long as we also have a few old-style examples around too

@gaetanmaisse gaetanmaisse mentioned this pull request Feb 2, 2020
Closed
@gaetanmaisse
Copy link
Member Author

I open this PR because with these commits everything should be OK for Yarn 2 & Storybook compatibility on simple projects. ⚠️ For now @storybook/preset-create-react-app is not working and I haven't tried all addons.

@shilman I didn't find a way to use Yarn 2 on only some projects of our Yarn v1 workspace. As soon as this PR will be merged and released I will add an e2e test on Yarn 2 repo matching the scenario given in the issue.

Then I will work on:

  • Updating @storybook/preset-create-react-app. it's currently resolving react-script in node_modules/.bin/ directory (see here), so I need to add one more check for SB PnP user and access webpack config file directly in the .zip file with PnP API.
  • Checking that all addons are working with the alpha of SB 6.0 that will release this PR.
  • Finding a way of adding examples with Yarn 2 inside SB monorepo. Maybe creating some GitHub actions as it is done on Yarn 2 repo can be a good idea because it really mimics the users' behaviors: https://github.com/yarnpkg/berry/blob/master/.github/workflows/e2e-jest-workflow.yml

@gaetanmaisse gaetanmaisse marked this pull request as ready for review February 2, 2020 09:45
@shilman
Copy link
Member

shilman commented Feb 2, 2020 •
edited
Loading

@gaetanmaisse This is really awesome work -- will be so great to announce this as part of the 6.0 release. 🤩🤩🤩

I'm not tracking the yarn 2 situation closely, but based on my limited understanding, we will need to support both yarn 1 and 2 for the foreseeable future. Therefore here's my strawman proposal, assuming that yarn 2 will be the primary use case moving forward, and yarn 1 will be the secondary:

  1. Let's convert the monorepo to yarn 2.
  2. Let's use the storybookjs/deprecated-addons repo as a yarn 1 test case.

What do you think? Alternatively, if I got the primary/secondary cases wrong, we can flip the proposal.

NOTE: To clarify, what I'm proposing is followup work, and not required for this PR. 😃

shilman
shilman reviewed Feb 2, 2020
View reviewed changes
addons/contexts/package.json
@@ -41,7 +41,20 @@
"qs": "*",
"rax": "*",
"react": "*",
"vue": "*"
"vue": "*",
Copy link
Member

Choose a reason for hiding this comment

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

@gaetanmaisse this PR somehow got removed: #7675

Do you think I should reapply that first before this PR? Would it change this part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can keep it as it is and as it will be with this PR, from my point of view it's more semantically correct to have preact, rax and vue listed as peerDep as we are importing them in the source code of addon-context (in addons/contexts/preview/framework). Adding the peerDependenciesMeta info will prevent any npm/yarn warning about missing dep which was the main point of #7625 and #7625 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

OK cool

@gaetanmaisse
Copy link
Member Author

@shilman I like your proposal! I will add to my todo list to investigate it 🕵 .

It is maybe a bit too ambitious to migrate our workspace to Yarn 2 right now because:

  • we don't have any feedback of big projects using it
  • I'm only playing with it on small personal projects for a few days

Anyway, I will take a look at what is possible and I will share my feedback and thoughts 😉

shilman
shilman approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM. Could use confirmation from @ndelangen or somebody else who's more familiar with the situation.

@gaetanmaisse
feat(core): create a function to resolve path of a file/dir in storyb…
8b4fe4f 
…ook cache directory

`find-cache-dir` is not used anymore because it returns `undefined` when there is no `node_modules` folder at project root, as in projects managed with Yarn 2.
@gaetanmaisse
refactor(core): avoid duplication of code used to get path of files/d…
b46444e 
…irs in cache
@gaetanmaisse
feat(core): move webpack default output from storybook/core own pac…
8af8666 
…kage folder to SB cache folder

Using Yarn 2 packages are read only, and more globally it's better to not alter the package with its own execution, so the output is moved outside the packages.
@gaetanmaisse
feat(core): update webpack config to handle Yarn PnP mode out of the box
782bf45 
Add PnpWebpackPlugin in manager and preview webpack configurations
@gaetanmaisse
chore: add missing or fix implicit transitive peerDependencies
a978bd2 
Yarn 2 introduce an error for this during `yarn install`: YN0002 - MISSING_PEER_DEPENDENCY

For details see https://dev.to/arcanis/implicit-transitive-peer-dependencies-ed0

TL;DR: If you write a package that depends on Foo, and if Foo has a peer dependency, then you must provide it in either of the dependencies or peerDependencies fields. You won't "implicitly inherit" the peer dependencies declared in Foo.
@shilman shilman added this to the 6.0.0 milestone Feb 4, 2020
@shilman shilman changed the title Add Yarn 2 compatibility Core: Add Yarn 2 compatibility Feb 4, 2020
@shilman shilman merged commit 05e66cb into next Feb 4, 2020
@shilman shilman deleted the add-yarn-2-compatibility branch February 4, 2020 08:43
This was referenced Feb 24, 2020
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@gaetanmaisse gaetanmaisse mentioned this pull request Feb 25, 2020
Closed
@voxpelli voxpelli mentioned this pull request Apr 14, 2020
Closed
@gaetanmaisse gaetanmaisse mentioned this pull request Jun 11, 2020
Closed
@dependencies dependencies bot mentioned this pull request Jan 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shilman shilman shilman approved these changes

@alexandrebodin alexandrebodin Awaiting requested review from alexandrebodin

@alterx alterx Awaiting requested review from alterx

@CodeByAlex CodeByAlex Awaiting requested review from CodeByAlex

@igor-dv igor-dv Awaiting requested review from igor-dv

@jbovenschen jbovenschen Awaiting requested review from jbovenschen

@kazupon kazupon Awaiting requested review from kazupon

@leonrodenburg leonrodenburg Awaiting requested review from leonrodenburg

@thani-sh thani-sh Awaiting requested review from thani-sh

@ndelangen ndelangen Awaiting requested review from ndelangen

@renaudtertrais renaudtertrais Awaiting requested review from renaudtertrais

@saponifi3d saponifi3d Awaiting requested review from saponifi3d

@theinterned theinterned Awaiting requested review from theinterned

@thomasbertet thomasbertet Awaiting requested review from thomasbertet

@tmeasday tmeasday Awaiting requested review from tmeasday

@usulpro usulpro Awaiting requested review from usulpro

@z4o4z z4o4z Awaiting requested review from z4o4z

Assignees
No one assigned
Labels
compatibility with other tools feature request yarn / npm Yarn / npm acting weird
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@gaetanmaisse @shilman