-
Notifications
You must be signed in to change notification settings - Fork 104
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
Make preset-create-react-app compatible with Yarn PnP/Yarn 2 #104
Conversation
048fae1
to
5c60824
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @gaetanmaisse. Just a few small notes, let me know if you have any questions.
"typescript": "^3.7.5" | ||
}, | ||
"peerDependencies": { | ||
"@storybook/react": ">=5.2", | ||
"react-scripts": ">=3.0.0" | ||
"node-sass": "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a dependency of react-scripts
if you want to use Sass, and is not related to this preset in my opinion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I added at the beginning of my work on Yarn 2 compatibility but cannot find any reason to keep it. I added a fixup commit and will rebase the branch properly after the PR gets validated ;)
@@ -0,0 +1 @@ | |||
declare module 'pnp-webpack-plugin'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this in an @types
folder instead, named the same as the missing types?
// When operating under PnP environments, this value will be set to a number | ||
// indicating the version of the PnP standard, see: https://yarnpkg.com/advanced/pnpapi#processversionspnp | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const IS_USING_YARN_PNP = (process.versions as any).pnp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try very hard to avoid any
in this codebase - can you try to define the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be converted to a boolean, as you aren't using the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to improve this by extending NodeJS type, with that no more any
. I also improved the check used for IS_USING_YARN_PNP
@mrmckeb Thanks for your feedback! I fixed them in a few more commits, however, I can't remote: Resolving deltas: 100% (14/14), completed with 4 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/add-yarn-2-support.
remote: error: 8 of 8 required status checks are expected.
To https://github.com/storybookjs/presets.git
! [remote rejected] add-yarn-2-support -> add-yarn-2-support (protected branch hook declined)
error: impossible de pousser des références vers 'https://github.com/storybookjs/presets.git' I'm not admin for this repo, so can you check in the settings if this branch is listed as protected? Or anything else that can explain this error 🙃 |
Sorry, there were some changes that @shilman and I were making yesterday. When this PR is merged, we'll have a new CI - I think the check requirements were accidentally applied to all branches. I'll try to look this morning with @shilman. Maybe subscribe to that PR, then when it's merged you can rebase and we can merge this in. Again, I'll work on branch protection today) |
@mrmckeb new commits pushed 🚀! |
Thanks @gaetanmaisse. We're still waiting for this to merge unfortunately, I'll chase @shilman again :) |
Hi @gaetanmaisse, this has finally been merged - if you can rebase/solve conflicts, I think we can get this out! |
@mrmckeb, great! I will do it as soon I have time too and tag you back 😉 |
Use Plug'n'Play API to find `react-scripts` package location directly with the dependency tree. See https://yarnpkg.com/advanced/pnpapi
…process.versions`
a3d1276
to
53fae42
Compare
@mrmckeb rebase done ✅ |
Related to storybookjs/storybook#9527.
What I did
react-scripts
package's location directly with the dependency tree. See https://yarnpkg.com/advanced/pnpapi 🚅How to test
Unfortunately, there is no great way to test it for now 😏. Locally:
Checkout this branch and build
@storybook/preset-create-react-app
, then in a tmp folder:Link
preset-create-react-app
package to the local version, to do so add inpackage.json
:Finally, run
yarn storybook
🎉