React: Add fallback for automatic JSX runtime detection #13899
Conversation
…solve to make yarn pnp work Signed-off-by: Aleksei Khudiakov <[email protected]>
| try { | ||
| require.resolve('react', { paths: [context] }) | ||
| } catch (e) { | ||
| require.resolve('react/jsx-runtime'); | ||
| return true; | ||
| } | ||
| require.resolve('react/jsx-runtime', { paths: [context] }); |
There was a problem hiding this comment.
| try { | |
| require.resolve('react', { paths: [context] }) | |
| } catch (e) { | |
| require.resolve('react/jsx-runtime'); | |
| return true; | |
| } | |
| require.resolve('react/jsx-runtime', { paths: [context] }); | |
| require.resolve('react/jsx-runtime', { paths: [context, __dirname] }); |
There was a problem hiding this comment.
pnp does not use node_modules directories for resolution. Package location can't be assumed (see .pnp.cjs example below). Using context paths here simply prevents pnp from resolving the package.
Initially I intended to use pnpapi to lookip the package that owns the path (not the node_modules but the location with .storybook/main.js) and from the context of that package lookup react/jsx-runtime, which would resolve to react instance required by user.
However, I see no way to get storybook application location from here.
So, the option I opted for is to attempt to resolve react first and if it can't be resolved at all then attempt to resolve without context, so pnp can kick in. This will always resolve to the react required by storybook but, still, it is better than automatic JSX detection not working at all for yarn pnp.
Normally, if react can be resolved (with npm or yarn v1) as originally implemented with context then it can attempt to resolve jsx runtime from that found react package.
Take note of the packageLocation field in this .pnp.cjs excerpt:
[
'@storybook/react',
[
[
'patch:@storybook/react@npm%3A6.1.18#./patches/storybook-jsx.diff::version=6.1.18&hash=35bebc&locator=%40privateorg%2Fworkspace%40workspace%3A.',
{
'packageLocation': './.yarn/cache/@storybook-react-patch-976f36eb94-3b088adc95.zip/node_modules/@storybook/react/',
'packageDependencies': [
[
'@storybook/react',
'patch:@storybook/react@npm%3A6.1.18#./patches/storybook-jsx.diff::version=6.1.18&hash=35bebc&locator=%40privateorg%2Fworkspace%40workspace%3A.'
]
],
'linkType': 'SOFT'
}
],
],
]$ yarn info -A -R @storybook/react
└─ @storybook/react@patch:@storybook/react@npm%3A6.1.18#./patches/storybook-jsx.diff::version=6.1.18&hash=35bebc&locator=%40privateorg%2Fworkspace%40workspace%3A.
├─ Instances: 1
├─ Version: 6.1.18
│
├─ Exported Binaries
│ ├─ build-storybook
│ ├─ start-storybook
│ └─ storybook-server
│
└─ Dependencies
├─ @babel/preset-flow@npm:^7.12.1 → npm:7.12.13
├─ @babel/preset-react@npm:^7.12.1 → npm:7.12.13
├─ @pmmmwh/react-refresh-webpack-plugin@npm:^0.4.2 → npm:0.4.3
├─ @storybook/addons@npm:6.1.18 → npm:6.1.18
├─ @storybook/core@npm:6.1.18 → npm:6.1.18
├─ @storybook/node-logger@npm:6.1.18 → npm:6.1.18
├─ @storybook/semver@npm:^7.3.2 → npm:7.3.2
├─ @types/webpack-env@npm:^1.15.3 → npm:1.16.0
├─ babel-plugin-add-react-displayname@npm:^0.0.5 → npm:0.0.5
├─ babel-plugin-named-asset-import@npm:^0.3.1 → npm:0.3.7
├─ babel-plugin-react-docgen@npm:^4.2.1 → npm:4.2.1
├─ core-js@npm:^3.0.1 → npm:3.8.3
├─ global@npm:^4.3.2 → npm:4.4.0
├─ lodash@npm:^4.17.15 → npm:4.17.20
├─ prop-types@npm:^15.7.2 → npm:15.7.2
├─ react-dev-utils@npm:^10.0.0 → npm:10.2.1
├─ react-docgen-typescript-plugin@npm:^0.6.2 → npm:0.6.3
├─ react-refresh@npm:^0.8.3 → npm:0.8.3
├─ regenerator-runtime@npm:^0.13.7 → npm:0.13.7
├─ ts-dedent@npm:^2.0.0 → npm:2.0.0
└─ webpack@npm:^4.44.2 → npm:4.46.0
There was a problem hiding this comment.
The example has patched version with this change applied locally. Unpatched version is next to it in .yarn/cache/@storybook-react-npm-6.1.18-e398bb22b4-f1d86f8c2c.zip No way that can be resolved with existing code.
There was a problem hiding this comment.
I'm well aware of how PnP works, the solution here is to change what the context is, it should be the file that is getting transformed as shown in #12899 (comment)
shilman
changed the title
Xerkus
changed the title
|
The title should be changed as this isn't related to Yarn PnP, it's just a bug in general |
|
Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks! |
shilman
changed the title
☁️ Nx Cloud ReportCI is running/has finished running commands for commit b29729e. 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. |
Issue: #12881
This is an attempt to fix new JSX detection when used with yarn pnp, since current approach using directory context can't work with pnp.
Detection needs to resolve user's react and not the one provided by storybook. However where it is invoked I see no way to get package context where storybook is used: pnpapi top level locator is always a root workspace package in yarn workspaces setup and locator roots include all the packages in the workspace with no way to detect which one is a proper root we need. Traversing parent modules would lead to root workspace package again with no mention of the package that requested our storybook dependency.
webpackFinal()might have meaningful value inconfig.filenamefor a pnp specific lookup, may be?React as peer dependency could turn out fine So, as it is, I suggest this workable approach where we fallback to no-context lookup for
react/jsx-runtimewhen we fail to resolvereactwith the directory context. That would always be the case for pnp resolution.This should fix the remaining issue with #12881