Open fluiddot opened 1 year ago
kevin940726
commented
1 year ago @storybook/addon-knobs has been deprecated. I guess we'll have to migrate it to @storybook/addon-controls instead. A temporary solution will be to set legacy-peer-deps to true in the .npmrc config.
mirka
commented
1 year ago I guess we'll have to migrate it to
@storybook/addon-controlsinstead.
We've been chipping away at this for a while (#35665) and we're down to just 11 more files. I'll try and get this prioritized so we can get it over the finish line. Please go with the temporary solution if it's urgent though.
ciampo
commented
1 year ago As of https://github.com/WordPress/gutenberg/pull/47152, the @storybok/addon-knobs dependency has been removed from the repo 🎉
@fluiddot , can you still replicate the issue or has it been fixed?
fluiddot
commented
1 year ago As of #47152, the
@storybok/addon-knobsdependency has been removed from the repo 🎉@fluiddot , can you still replicate the issue or has it been fixed?
Thanks for the call out @ciampo 🙇 , I haven't tested this after the recent fixes. Let me do another check and get back here with the results.
fluiddot
commented
1 year ago I tried to install the dependencies again with npm 8 but found new issues:
@testing-library/react-native dependencyError:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: @testing-library/react-native@11.3.0
npm ERR! Found: jest@27.4.5
npm ERR! node_modules/jest
npm ERR! dev jest@"27.4.5" from the root project
npm ERR! peer jest@"^27.0.0" from jest-watch-typeahead@1.0.0
npm ERR! node_modules/jest-watch-typeahead
npm ERR! dev jest-watch-typeahead@"1.0.0" from the root project
npm ERR! 7 more (snapshot-diff, @wordpress/e2e-test-utils, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peerOptional jest@">=28.0.0" from @testing-library/react-native@11.3.0
npm ERR! node_modules/@testing-library/react-native
npm ERR! dev @testing-library/react-native@"11.3.0" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: jest@29.4.0
npm ERR! node_modules/jest
npm ERR! peerOptional jest@">=28.0.0" from @testing-library/react-native@11.3.0
npm ERR! node_modules/@testing-library/react-native
npm ERR! dev @testing-library/react-native@"11.3.0" from the root projectThis can be easily fixed by simply bumping the version of the dependency to 11.5.0.
Error:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: gutenberg@15.0.1
npm ERR! Found: react@18.2.0
npm ERR! node_modules/react
npm ERR! dev react@"18.2.0" from the root project
npm ERR! peer react@">=16.0.0" from @testing-library/react-native@11.5.0
npm ERR! node_modules/@testing-library/react-native
npm ERR! dev @testing-library/react-native@"11.5.0" from the root project
npm ERR! 28 more (@wordpress/block-directory, @wordpress/block-editor, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer react@"18.0.0" from react-native@0.69.4
npm ERR! node_modules/react-native
npm ERR! dev react-native@"0.69.4" from the root project
npm ERR! peer react-native@">=0.59" from @testing-library/react-native@11.5.0
npm ERR! node_modules/@testing-library/react-native
npm ERR! dev @testing-library/react-native@"11.5.0" from the root project
npm ERR! 3 more (@wordpress/react-native-aztec, ...)After fixing the issue in @testing-library/react-native dependency, I bumped into the above error. As far as I investigated, seems we use a higher version of React (18.2.0) than the one supported by React Native (18.0.0). This version was set in https://github.com/WordPress/gutenberg/pull/45235 and didn't produce any side issues, but looks like npm 8 is more strict with the dependency resolution.
I foresee we have the following options to solve this:
18.0.0.0.71.0 as this is the one where the React dependency was updated to 18.2.0(reference).--legacy-peer-deps parameter when installing the dependencies.Probably the second one is the most optimal but we currently don't have enough bandwidth to execute another React Native upgrade in the Gutenberg Mobile project, especially as we did one a month ago. The third one I understand is a no-go because disrupts the development workflow, so we might consider going with the first one temporarily. WDYT?
cc @jsnajdr @noahtallen @tyxla @geriux as participants in https://github.com/WordPress/gutenberg/pull/45235 PR
noahtallen
commented
1 year ago It looks like we would miss out on a lot of bug fixes: https://github.com/facebook/react/blob/main/CHANGELOG.md I'm not sure if any of these were critical to getting React working in Gutenberg, but it'd definitely be nice to have them. Out of curiosity, when would we expect to be able to do another React Native upgrade? (Though I do know react native is a huge pain to upgrade!)
I also see in this PR (#46576) that legacy-peer-deps could be enabled for the time being, just so there is a base to work off of. While not ideal in the long-run, maybe ok for now as we get everything updated and start chipping away at peer dependency issues?
All that said, in my experience with these types of projects (where we start to enforce peer dependencies), we've generally had to include some way to override some broken peer dependencies which we manually verify. For example Yarn has the .yarnrc.yml file to accomplish this, along with a "resolutions" feature and a built-in way to patch installed packages. I think pnpm has a similar feature.
I think NPM has some similar features too, so I wonder if we could override the React version required by React Native with overrides? https://docs.npmjs.com/cli/v8/configuring-npm/package-json?v=true#overrides
Obviously we'd need to test that react native works, but could be worth it in this case. There will probably be some similar packages we rely on which haven't yet declared support for React 18, or even React 17.
I guess the main downside is that you're not following what the library technically supports, but in this case, it seems like React Native is probably just very cautious rather than explicitly saying React 18.2 isn't supported.
jsnajdr
commented
1 year ago React Native did the peer dependency update from 18.0.0 to 18.2.0 in this commit. It doesn't just update the dependency, but also synchronizes many changes from React to React Native's renderer. Which is the mobile equivalent of the react-dom package.
So, our react-native situation is like using react@18.2.0 together with react-dom@18.0.0. Is that harmful? Potentially yes, but in this actual case it's not: when I look at the diff between react@18.0.0 and react@18.2.0, there are no changes at all.
How is that possible? It's because in React all the hard work is done by the react-dom package, or the react-native one. The react package is really tiny, 6kB minified, it contains code for creating elements, defines constants for various element types, and has trivial implementation of hooks that look like:
function useState(initialState) {
var dispatcher = resolveDispatcher();
return dispatcher.useState(initialState);
}This code hasn't changed between 18.0.0 and 18.2.0.
So the conclusion is that we can safely override or ignore the 18.0.0 peer dependency.
Downgrade the React version in Gutenberg to 18.0.0.
This doesn't look like a good tradeoff to me: downgrading React, which has already been shipped in the Gutenberg plugin and will be shipped in Core soon, and which powers user-visible features, only to be able to upgrade NPM, which is a build tool invisible outside the Gutenberg repo and doesn't affect any published artifacts?
I'd rather either postpone the NPM upgrade, or do the React Native upgrade to unblock it.
we currently don't have enough bandwidth to execute another React Native upgrade in the Gutenberg Mobile project
Do we know what exactly the React Native upgrade from 0.69.4 to 0.71.1 would mean? Is it only a matter of the Gutenberg repo, or does it spread also to other wordpress-mobile/* projects? On our team we are kind of specialists on upgrades and migrations, so maybe we could help 🙂
fluiddot
commented
1 year ago Thanks @noahtallen @jsnajdr for checking this, I really appreciate it 🙇 .
I also see in this PR (https://github.com/WordPress/gutenberg/pull/46576) that legacy-peer-deps could be enabled for the time being, just so there is a base to work off of. While not ideal in the long-run, maybe ok for now as we get everything updated and start chipping away at peer dependency issues?
Yep, this could be a temporary solution until we fix the dependency issues.
I think NPM has some similar features too, so I wonder if we could override the React version required by React Native with overrides? https://docs.npmjs.com/cli/v8/configuring-npm/package-json?v=true#overrides
I tried using the overrides configuration for the dependencies that failed and it worked 🎊 . So this could be a better option than using legacy-peer-deps, although it would still be a temporary solution.
So, our
react-nativesituation is like usingreact@18.2.0together withreact-dom@18.0.0. Is that harmful? Potentially yes, but in this actual case it's not: when I look at the diff betweenreact@18.0.0andreact@18.2.0, there are no changes at all.How is that possible? It's because in React all the hard work is done by the
react-dompackage, or thereact-nativeone. Thereactpackage is really tiny, 6kB minified, it contains code for creating elements, defines constants for various element types, and has trivial implementation of hooks that look like:function useState(initialState) { var dispatcher = resolveDispatcher(); return dispatcher.useState(initialState); }This code hasn't changed between 18.0.0 and 18.2.0.
So the conclusion is that we can safely override or ignore the 18.0.0 peer dependency.
Interesting results, thank you very much @jsnajdr for investigating this. With this information, seems that the option of overriding the dependencies would work until we provide a fix.
This doesn't look like a good tradeoff to me: downgrading React, which has already been shipped in the Gutenberg plugin and will be shipped in Core soon, and which powers user-visible features, only to be able to upgrade NPM, which is a build tool invisible outside the Gutenberg repo and doesn't affect any published artifacts?
Good point, let's discard this option.
I'd rather either postpone the NPM upgrade, or do the React Native upgrade to unblock it.
Postponing NPM upgrade can't be risky as the currently supported Node version (node 14) is in maintenance and the End-of-life is April, 30th, 2023. Hence, we'd need to upgrade React Native before that date. We could try to prioritize the upgrade in the Gutenberg Mobile project but most of the bandwidth is focused on other projects, so this option can be a bit tight in the schedule.
Being this said, I'll prepare a PR as a follow-up on these comments and to address this issue using the option of overriding the dependencies.
Following my previous comment, I'd like to make an update because I spotted more issues when installing the dependencies:
@testing-library/react-nativejest@">=28.0.0"jest@27.4.5I see there's an ongoing effort to upgrade Jest version to 29 in https://github.com/WordPress/gutenberg/pull/47388, so we can either downgrade the dependency @testing-library/react-native to a previous version (10.1.1) or wait to the PR to be merged.
react-nativereact@"18.0.0"react@18.2.0As discussed above, the optimal approach for this would be to upgrade React Native to version 0.71.0. However, in the meantime, we could override the dependency as a workaround.
storybook-source-linkreact@"^16.8.0 || ^17.0.0"react@18.2.0react-dom@"^16.8.0 || ^17.0.0"react-dom@18.2.0Looks like this issue could be resolved by upgrading the dependency to version 2.0.4 that allows React 18 as a peer dependency.
cc @mirka as author of the PR that introduced this dependency, in case we could bump the version. Thanks!
I undestand that these warnings won't block using npm 8 but it would be interesting to address them in the future.
Affected dependencies:
@mdx-js/reactajv-keywordsreact-element-to-jsx-stringreact-inspectorwebpack-filter-warnings-pluginmirka
commented
1 year ago
storybook-source-link
Done in #47425 👍
Description
Part of Upgrade node and npm to latest LTS versions.
Related to https://github.com/WordPress/gutenberg/pull/46400 and now that https://github.com/WordPress/gutenberg/issues/40540 is solved, I tried installing the dependencies using npm
8.19.3(node version14.20.1). The first time I rannpm iit worked but the second time I encountered the following error:Based on the error logs, looks like there's a dependency conflict in
@storybook/addon-knobswith the React versions.Step-by-step reproduction instructions
npm i -g npm@8).npm i.npm iagain.Screenshots, screen recording, code snippet
No response
Environment info
No response
Please confirm that you have searched existing issues in the repo.
Yes
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Yes