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

React: Add fallback for automatic JSX runtime detection #13899

Closed
wants to merge 2 commits into from
Closed

React: Add fallback for automatic JSX runtime detection #13899

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1e2599
Update JSX automatic runtime detection to fallback to context-less re…
Xerkus Feb 13, 2021
b29729e
Merge branch 'next' into feature/react-jsx-detection-with-pnp
ndelangen Jun 30, 2022
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
6 changes: 6 additions & 0 deletions app/react/src/server/framework-preset-react.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ const context = storybookReactDirName.includes('node_modules')

const hasJsxRuntime = () => {
try {
try {
require.resolve('react', { paths: [context] })
} catch (e) {
require.resolve('react/jsx-runtime');
return true;
}
require.resolve('react/jsx-runtime', { paths: [context] });
Comment on lines +41 to 47
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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] });

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

return true;
} catch (e) {
Expand Down