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.

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

Addon Docs: Remove react peer dependency #24881

Merged
merged 27 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
376029f
spike removing react as a peer dependency from addon-docs
JReinhold Nov 16, 2023
5317302
Merge branch 'next' of github.com:storybookjs/storybook into addon-do…
JReinhold Nov 16, 2023
7f6c1cc
update lockfile
JReinhold Nov 17, 2023
c85a698
remove unused join
JReinhold Nov 17, 2023
012e909
fix
JReinhold Nov 17, 2023
5982486
Merge branch 'next' into addon-docs-without-react
ndelangen Dec 12, 2023
297eca8
cleanup
ndelangen Dec 12, 2023
f5a7235
cleanup
ndelangen Dec 12, 2023
90e95bd
cleanup
ndelangen Dec 12, 2023
5d65a35
fixes
ndelangen Dec 12, 2023
c44d4ee
fixes
ndelangen Dec 12, 2023
c49a841
fix react-dom-shim
JReinhold Dec 12, 2023
e2374e5
Merge branches 'addon-docs-without-react' and 'addon-docs-without-rea…
JReinhold Dec 12, 2023
171493e
add e2e test for resolved react version
JReinhold Dec 13, 2023
c590019
fix react-dom-shim in vite too
JReinhold Dec 13, 2023
dab5b63
add react alias to beginning of array
JReinhold Dec 13, 2023
e5b8f86
Merge branch 'next' into addon-docs-without-react
ndelangen Dec 13, 2023
dab576d
react alias plugin enforce pre
JReinhold Dec 13, 2023
f81a397
fix ResolvedReactVersion tsx
JReinhold Dec 13, 2023
46cb279
Merge branch 'addon-docs-without-react' of github.com:storybookjs/sto…
JReinhold Dec 13, 2023
d0979bc
fix e2e test asserting react 17
JReinhold Dec 13, 2023
6ea2365
don't attempt to resolve @mdx-js/react outside of addon-docs
JReinhold Dec 14, 2023
99a1b78
fix
ndelangen Dec 14, 2023
01182e5
cleanup
JReinhold Dec 15, 2023
00f031b
Merge branch 'addon-docs-without-react' of github.com:storybookjs/sto…
JReinhold Dec 15, 2023
28051ed
Merge branch 'next' into addon-docs-without-react
JReinhold Dec 15, 2023
2db44d7
Merge branch 'next' into addon-docs-without-react
JReinhold Dec 18, 2023
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
8 changes: 2 additions & 6 deletions code/addons/docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,21 +112,17 @@
"@storybook/theming": "workspace:*",
"@storybook/types": "workspace:*",
"fs-extra": "^11.1.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"remark-external-links": "^8.0.0",
"remark-slug": "^6.0.0",
"ts-dedent": "^2.0.0"
},
"devDependencies": {
"@rollup/pluginutils": "^5.0.2",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"typescript": "^5.3.2",
"vite": "^4.0.4"
},
"peerDependencies": {
"react": "^16.8.0 || ^17.0.0 || ^18.0.0",
"react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0"
},
"publishConfig": {
"access": "public"
},
Expand Down
40 changes: 0 additions & 40 deletions code/addons/docs/src/ensure-react-peer-deps.ts

This file was deleted.

87 changes: 84 additions & 3 deletions code/addons/docs/src/preset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,26 @@ import type { JSXOptions, CompileOptions } from '@storybook/mdx2-csf';
import { global } from '@storybook/global';
import { loadCsf } from '@storybook/csf-tools';
import { logger } from '@storybook/node-logger';
import { ensureReactPeerDeps } from './ensure-react-peer-deps';

/**
* Get the resolvedReact preset, which points either to
* the user's react dependencies or the react dependencies shipped with addon-docs
* if the user has not installed react explicitly.
*/
const getResolvedReact = async (options: Options) => {
const resolvedReact = (await options.presets.apply('resolvedReact', {})) as any;
// resolvedReact should always be set by the time we get here, but just in case, we'll default to addon-docs's react dependencies
return {
react: resolvedReact.react ?? dirname(require.resolve('react/package.json')),
reactDom: resolvedReact.reactDom ?? dirname(require.resolve('react-dom/package.json')),
// In Webpack, symlinked MDX files will cause @mdx-js/react to not be resolvable if it is not hoisted
// This happens for the SB monorepo's template stories when a sandbox has a different react version than
// addon-docs, causing addon-docs's dependencies not to be hoisted.
// This might also affect regular users who have a similar setup.
// Explicitly alias @mdx-js/react to avoid this issue.
mdx: resolvedReact.mdx ?? dirname(require.resolve('@mdx-js/react/package.json')),
};
};

async function webpack(
webpackConfig: any = {},
Expand Down Expand Up @@ -90,6 +109,35 @@ async function webpack(
? require.resolve('@storybook/mdx1-csf/loader')
: require.resolve('@storybook/mdx2-csf/loader');

// Use the resolvedReact preset to alias react and react-dom to either the users version or the version shipped with addon-docs
const { react, reactDom, mdx } = await getResolvedReact(options);

let alias;
if (Array.isArray(webpackConfig.resolve?.alias)) {
alias = [...webpackConfig.resolve?.alias];
alias.push(
{
name: 'react',
alias: react,
},
{
name: 'react-dom',
alias: reactDom,
},
{
name: '@mdx-js/react',
alias: mdx,
}
);
} else {
alias = {
...webpackConfig.resolve?.alias,
react,
'react-dom': reactDom,
'@mdx-js/react': mdx,
};
}

const result = {
...webpackConfig,
plugins: [
Expand All @@ -99,7 +147,10 @@ async function webpack(
? [(await import('@storybook/csf-plugin')).webpack(csfPluginOptions)]
: []),
],

resolve: {
...webpackConfig.resolve,
alias,
},
module: {
...module,
rules: [
Expand Down Expand Up @@ -179,6 +230,25 @@ export const viteFinal = async (config: any, options: Options) => {
const { plugins = [] } = config;
const { mdxPlugin } = await import('./plugins/mdx-plugin');

// Use the resolvedReact preset to alias react and react-dom to either the users version or the version shipped with addon-docs
const { react, reactDom } = await getResolvedReact(options);

const reactAliasPlugin = {
name: 'storybook:react-alias',
enforce: 'pre',
config: () => ({
resolve: {
alias: {
react,
'react-dom': reactDom,
},
},
}),
};

// add alias plugin early to ensure any other plugins that also add the aliases will override this
// eg. the preact vite plugin adds its own aliases
plugins.unshift(reactAliasPlugin);
plugins.push(mdxPlugin(options));

return config;
Expand All @@ -192,7 +262,18 @@ const webpackX = webpack as any;
const indexersX = indexers as any;
const docsX = docs as any;

ensureReactPeerDeps();
/**
* If the user has not installed react explicitly in their project,
* the resolvedReact preset will not be set.
* We then set it here in addon-docs to use addon-docs's react version that always exists.
* This is just a fallback that never overrides the existing preset,
* but ensures that there is always a resolved react.
*/
export const resolvedReact = async (existing: any) => ({
react: existing?.react ?? dirname(require.resolve('react/package.json')),
reactDom: existing?.reactDom ?? dirname(require.resolve('react-dom/package.json')),
mdx: existing?.mdx ?? dirname(require.resolve('@mdx-js/react/package.json')),
});

const optimizeViteDeps = [
'@mdx-js/react',
Expand Down
13 changes: 13 additions & 0 deletions code/addons/docs/template/stories/docs2/ResolvedReact.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { version as reactVersion } from 'react';
import { version as reactDomVersion } from 'react-dom';
import { ResolvedReactVersion } from './ResolvedReactVersion';

## In MDX

<code>react</code>: <code data-testid="mdx-react">{reactVersion}</code>

<code>react-dom</code>: <code data-testid="mdx-react-dom">{reactDomVersion}</code>

## In `ResolvedReactVersion` component

<ResolvedReactVersion />
15 changes: 15 additions & 0 deletions code/addons/docs/template/stories/docs2/ResolvedReactVersion.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React, { version as reactVersion } from 'react';
import { version as reactDomVersion } from 'react-dom';

export const ResolvedReactVersion = () => {
return (
<>
<p>
<code>react</code>: <code data-testid="component-react">{reactVersion}</code>
</p>
<p>
<code>react-dom</code>: <code data-testid="component-react-dom">{reactDomVersion}</code>
</p>
</>
);
};
4 changes: 0 additions & 4 deletions code/addons/essentials/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,6 @@
"devDependencies": {
"typescript": "^5.3.2"
},
"peerDependencies": {
"react": "^16.8.0 || ^17.0.0 || ^18.0.0",
"react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0"
},
"publishConfig": {
"access": "public"
},
Expand Down
27 changes: 27 additions & 0 deletions code/e2e-tests/addon-docs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,4 +186,31 @@ test.describe('addon-docs', () => {
await expect(stories.nth(1)).toHaveText('Basic');
await expect(stories.last()).toHaveText('Another');
});

test('should resolve react to the correct version', async ({ page }) => {
const sbPage = new SbPage(page);
await sbPage.navigateToUnattachedDocs('addons/docs/docs2', 'ResolvedReact');
const root = sbPage.previewRoot();

let expectedReactVersion = /^18/;
if (
templateName.includes('preact') ||
templateName.includes('react-webpack/17') ||
templateName.includes('react-vite/17')
) {
expectedReactVersion = /^17/;
} else if (templateName.includes('react16')) {
expectedReactVersion = /^16/;
}

const mdxReactVersion = await root.getByTestId('mdx-react');
const mdxReactDomVersion = await root.getByTestId('mdx-react-dom');
const componentReactVersion = await root.getByTestId('component-react');
const componentReactDomVersion = await root.getByTestId('component-react-dom');

await expect(mdxReactVersion).toHaveText(expectedReactVersion);
await expect(mdxReactDomVersion).toHaveText(expectedReactVersion);
await expect(componentReactVersion).toHaveText(expectedReactVersion);
await expect(componentReactDomVersion).toHaveText(expectedReactVersion);
});
});
20 changes: 20 additions & 0 deletions code/e2e-tests/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,26 @@ export class SbPage {
await this.previewRoot();
}

async navigateToUnattachedDocs(title: string, name = 'docs') {
await this.openComponent(title);

const titleId = toId(title);
const storyId = toId(name);
const storyLinkId = `#${titleId}-${storyId}--docs`;
await this.page.waitForSelector(storyLinkId);
const storyLink = this.page.locator('*', { has: this.page.locator(`> ${storyLinkId}`) });
await storyLink.click({ force: true });

await this.page.waitForURL((url) =>
url.search.includes(`path=/docs/${titleId}-${storyId}--docs`)
);

const selected = await storyLink.getAttribute('data-selected');
await expect(selected).toBe('true');

await this.previewRoot();
}

async waitUntilLoaded() {
// make sure we start every test with clean state – to avoid possible flakyness
await this.page.context().addInitScript(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function Component() {
name: 'Prefetch',
},
{
// @ts-expect-error (a legacy nextjs api?)
cb: () => router.push('/push-html', { forceOptimisticNavigation: true }),
name: 'Push HTML',
},
Expand All @@ -32,6 +33,7 @@ function Component() {
name: 'Refresh',
},
{
// @ts-expect-error (a legacy nextjs api?)
cb: () => router.replace('/replaced-html', { forceOptimisticNavigation: true }),
name: 'Replace',
},
Expand Down
20 changes: 19 additions & 1 deletion code/lib/core-server/src/presets/common-preset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import type {
PresetProperty,
} from '@storybook/types';
import { printConfig, readConfig, readCsf } from '@storybook/csf-tools';
import { join, isAbsolute } from 'path';
import { join, dirname, isAbsolute } from 'path';
import { dedent } from 'ts-dedent';
import fetch from 'node-fetch';
import type { Channel } from '@storybook/channels';
Expand Down Expand Up @@ -344,3 +344,21 @@ export const experimental_serverChannel = async (

return channel;
};

/**
* Try to resolve react and react-dom from the root node_modules of the project
* addon-docs uses this to alias react and react-dom to the project's version when possible
* If the user doesn't have an explicit dependency on react this will return the existing values
* Which will be the versions shipped with addon-docs
*/
export const resolvedReact = async (existing: any) => {
try {
return {
...existing,
react: dirname(require.resolve('react/package.json')),
reactDom: dirname(require.resolve('react-dom/package.json')),
};
} catch (e) {
return existing;
}
};
39 changes: 27 additions & 12 deletions code/lib/react-dom-shim/src/preset.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
import type { Options } from '@storybook/types';
// @ts-expect-error react-dom doesn't have this in export maps in v16, messing up TS
import { version } from 'react-dom/package.json';
import { join, dirname } from 'path';
import { readFile } from 'fs/promises';

export const webpackFinal = async (config: any, options: Options) => {
/**
* Get react-dom version from the resolvedReact preset, which points to either
* a root react-dom dependency or the react-dom dependency shipped with addon-docs
*/
const getIsReactVersion18 = async (options: Options) => {
const { legacyRootApi } =
(await options.presets.apply<{ legacyRootApi?: boolean } | null>('frameworkOptions')) || {};

const isReact18 = version.startsWith('18') || version.startsWith('0.0.0');
const useReact17 = legacyRootApi ?? !isReact18;
if (!useReact17) return config;
if (legacyRootApi) {
return false;
}

const resolvedReact = await options.presets.apply<{ reactDom?: string }>('resolvedReact', {});
const reactDom = resolvedReact.reactDom || dirname(require.resolve('react-dom/package.json'));

const { version } = JSON.parse(await readFile(join(reactDom, 'package.json'), 'utf-8'));
return version.startsWith('18') || version.startsWith('0.0.0');
};

export const webpackFinal = async (config: any, options: Options) => {
const isReactVersion18 = await getIsReactVersion18(options);
if (isReactVersion18) {
return config;
}

return {
...config,
Expand All @@ -23,12 +40,10 @@ export const webpackFinal = async (config: any, options: Options) => {
};

export const viteFinal = async (config: any, options: Options) => {
const { legacyRootApi } =
(await options.presets.apply<{ legacyRootApi?: boolean } | null>('frameworkOptions')) || {};

const isReact18 = version.startsWith('18') || version.startsWith('0.0.0');
const useReact17 = legacyRootApi || !isReact18;
if (!useReact17) return config;
const isReactVersion18 = await getIsReactVersion18(options);
if (isReactVersion18) {
return config;
}

const alias = Array.isArray(config.resolve?.alias)
? config.resolve.alias.concat({
Expand Down
Loading