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

CLI: Add prompt-only automigrate asking for react-removal #25215

Merged
merged 7 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
158 changes: 158 additions & 0 deletions code/lib/cli/src/automigrate/fixes/prompt-remove-react.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
import type { StorybookConfig } from '@storybook/types';
import { glob } from 'glob';
import { removeReactDependency } from './prompt-remove-react';
import type { JsPackageManager } from '../../js-package-manager';

const check = async ({
packageManagerContent,
main: mainConfig,
storybookVersion = '8.0.0',
}: {
packageManagerContent: Pick<
Partial<Awaited<ReturnType<JsPackageManager['retrievePackageJson']>>>,
'dependencies' | 'devDependencies' | 'peerDependencies'
>;
main: Partial<StorybookConfig> & Record<string, unknown>;
storybookVersion?: string;
}) => {
const packageManager = {
retrievePackageJson: async () => packageManagerContent,
} as JsPackageManager;

return removeReactDependency.check({
packageManager,
configDir: '',
mainConfig: mainConfig as any,
storybookVersion,
});
};

jest.mock('glob', () => ({ glob: jest.fn(() => []) }));

describe('early exits', () => {
test('cancel if storybookVersion < 8', async () => {
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: [],
framework: '@storybook/vue-vite',
},
storybookVersion: '7.0.0',
})
).resolves.toBeFalsy();
});

test('cancel if no react deps', async () => {
await expect(
check({
packageManagerContent: {},
main: {
stories: [],
framework: '@storybook/vue-vite',
},
})
).resolves.toBeFalsy();
});

test('cancel if react renderer', async () => {
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: [],
framework: '@storybook/react-vite',
},
})
).resolves.toBeFalsy();

await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: [],
framework: '@storybook/nextjs',
},
})
).resolves.toBeFalsy();

await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: [],
framework: { name: '@storybook/react-webpack5' },
},
})
).resolves.toBeFalsy();
});
});

describe('prompts', () => {
test('simple', async () => {
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: ['*.stories.ts'],
addons: [],
framework: '@storybook/vue-vite',
},
})
).resolves.toEqual({ hasDocs: false, hasEssentials: false, hasMDX: false });
});
test('detects addon docs', async () => {
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: ['*.stories.ts'],
addons: ['@storybook/addon-docs'],
framework: '@storybook/vue-vite',
},
})
).resolves.toEqual({ hasDocs: true, hasEssentials: false, hasMDX: false });
});
test('detects addon essentials', async () => {
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: ['*.stories.ts'],
addons: ['@storybook/addon-docs', '@storybook/addon-essentials'],
framework: '@storybook/vue-vite',
},
})
).resolves.toEqual({ hasDocs: true, hasEssentials: true, hasMDX: false });
});
test('detects MDX usage', async () => {
// @ts-expect-error (jest mocked)
glob.mockImplementationOnce(() => ['*.stories.mdx']);
await expect(
check({
packageManagerContent: {
dependencies: { react: '16.0.0' },
},
main: {
stories: ['*.stories.ts'],
addons: ['@storybook/addon-docs', '@storybook/addon-essentials'],
framework: '@storybook/vue-vite',
},
})
).resolves.toEqual({ hasDocs: true, hasEssentials: true, hasMDX: true });
});
});
133 changes: 133 additions & 0 deletions code/lib/cli/src/automigrate/fixes/prompt-remove-react.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import dedent from 'ts-dedent';
import semver from 'semver';
import type { StoriesEntry } from 'lib/types/src';
import { commonGlobOptions, normalizeStories } from '@storybook/core-common';
import { isAbsolute, join, relative } from 'path';
import slash from 'slash';
import { glob } from 'glob';
import { getFrameworkPackageName } from '../helpers/mainConfigFile';
import type { Fix } from '../types';

interface Options {
hasMDX: boolean;
hasEssentials: boolean;
hasDocs: boolean;
}

async function detectMDXEntries(entries: StoriesEntry[], configDir: string): Promise<boolean> {
const list = normalizeStories(entries, {
configDir,
workingDir: configDir,
// defaultFilesPattern: '**/*.@(stories.@(js|jsx|mjs|ts|tsx))',
});
const result = (
await Promise.all(
list.map(async ({ directory, files, titlePrefix }) => {
const pattern = join(directory, files);
const absolutePattern = isAbsolute(pattern) ? pattern : join(configDir, pattern);
const absoluteDirectory = isAbsolute(directory) ? directory : join(configDir, directory);

return {
files: (
await glob(slash(absolutePattern), {
...commonGlobOptions(absolutePattern),
follow: true,
})
).map((f) => relative(absoluteDirectory, f)),
directory,
titlePrefix,
};
})
)
).reduce<boolean>((acc, { files }, i) => {
const filteredEntries = files.filter((s) => !s.endsWith('.mdx'));
if (filteredEntries.length < files.length) {
return true;
}
return acc;
}, false);
return result;
}

export const removeReactDependency: Fix<Options> = {
id: 'remove-react-dependency',

async check({ packageManager, mainConfig, storybookVersion, configDir }) {
let hasMDX = false;
let hasEssentials = false;
let hasDocs = false;

// when the user is using the react renderer, we should not prompt them to remove react
const frameworkPackageName = getFrameworkPackageName(mainConfig);
if (frameworkPackageName?.includes('react') || frameworkPackageName?.includes('nextjs')) {
return null;
}

// if the user has no dependency on react, we can skip this fix
const packageJson = await packageManager.retrievePackageJson();
if (
!packageJson?.dependencies?.['react'] &&
!packageJson?.peerDependencies?.['react'] &&
!packageJson?.devDependencies?.['react']
) {
return null;
}

// do not prompt to remove react for older versions of storybook
if (!semver.gte(storybookVersion, '8.0.0')) {
return null;
}

const { addons } = mainConfig;
hasEssentials = !!(
addons &&
addons.find((addon) =>
typeof addon === 'string'
? addon.endsWith('@storybook/addon-essentials')
: addon.name.endsWith('@storybook/addon-essentials')
)
);
hasDocs = !!(
addons &&
addons.find((addon) =>
typeof addon === 'string'
? addon.endsWith('@storybook/addon-docs')
: addon.name.endsWith('@storybook/addon-docs')
)
);

hasMDX = !!(await detectMDXEntries(mainConfig.stories, configDir || process.cwd()));

return {
hasMDX,
hasEssentials,
hasDocs,
};
},
prompt({ hasMDX, hasDocs, hasEssentials }) {
const addons = [hasEssentials ? 'essentials' : '', hasDocs ? 'docs' : ''].filter(Boolean);
const addonReasonText =
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like an unnecessary detail to me, I don't think it's needed.

addons.length > 0 ? `, because you are using ${addons.join(' and ')}` : '';

const start = dedent`
We detected that your project has a dependency for "react" that it might not need.
Nothing breaks by having it, you can safely ignore this message, if you wish.
ndelangen marked this conversation as resolved.
Show resolved Hide resolved

Storybook asked you to add "react" as a direct dependency in the past${addonReasonText}.
However, since version 8.0, Storybook no longer requires you to provide "react" as a dependency.
ndelangen marked this conversation as resolved.
Show resolved Hide resolved
`;

if (hasMDX) {
const mdxSuggestion = dedent`
As you are using '.mdx'-files, it might be reasonable to keep the dependency.
`;
return [start, mdxSuggestion].join('\n\n');
}
Copy link
Contributor

@valentinpalkovic valentinpalkovic Dec 13, 2023

Choose a reason for hiding this comment

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

The detectMDXEntries is pretty heavy and requires some heavy-lifting file system lookups. I don't think it's worth it just to log this one-liner to the user. What is the worst that can happen? Things might break and the user has to re-add react and react-dom. I think that's acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

When the user uses .mdx files, they will very likely want to keep the react dependency.

I pulled this code from the override preset, it costs like <80ms. Is that heavy lifting file system lookups to you?

Copy link
Contributor

@valentinpalkovic valentinpalkovic Dec 13, 2023

Choose a reason for hiding this comment

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

I don't know in which particular project you did the measurement, but I guess that the automigration might be blocking in bigger projects.

Also, I don't quite understand what you want to achieve:

  • You only scan for mdx files, if they are part of the stories glob. If they are not defined in the stories glob, you are not checking for them. So why not directly just search for mdx in the stories glob pattern? I don't think file system lookups are necessary in this case.
  • What about MDX usage outside of Storybook. This case isn't covered here, since if the stories glob pattern doesn't mention mdx files, they also subsequently will not be found by glob. So if you want to cover this scenario, you don't have to consider the stories glob at all but create a simple glob, which considers mdx files and pass it directly to the glob function. Also, isn't this case the only relevant one? We want to keep react and react-dom if e.g. MDX is used outside of Storybook. Isn't this the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't think this check is necessary at all. Early results in #24881 shows that it's perfectly valid to have MDX files - even ones importing React components - in a project without react and react-dom. It just works. TypeScript in the IDE might complain - I haven't tested that out yet - but I don't think that warrants this additional message.


const removalSuggestion = dedent`
We suggest you manually remove the dependency from your project.
We cannot do this automatically, removing it might break your project, so it should be done manually with care.
`;
return [start, removalSuggestion].join('\n\n');
},
};