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

Presets: Replace config with previewAnnotations, remove previewEntries #19152

Merged
merged 8 commits into from
Sep 13, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Sep 9, 2022

Issue: We realised that the handling of the previewEntries field in the vite build wasn't quite right (it was treating them identically to config entries, which have special behaviour on export).

Looking in the code base, we no longer use previewEntries (did we ever?), so decided to essentially pseudo-deprecate them by not supporting them in the Vite builder.

How to test

Check a vite sandbox.

@tmeasday tmeasday added maintenance User-facing maintenance tasks builder-vite labels Sep 9, 2022
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@@ -13,6 +13,7 @@ import {
import prompts from 'prompts';
import type { AbortController } from 'node-abort-controller';
import command from 'execa';
import dedent from 'ts-dedent';
Copy link
Member

Choose a reason for hiding this comment

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

We should add https://www.npmjs.com/package/@ianvs/prettier-plugin-sort-imports so we don't need to worry about manually organizing imports. :-D

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@tmeasday @IanVS

What's the difference between previewEntries and config? Is it that config exports are treated as annotations (e.g. parameters etc.)?

I think @ndelangen and I may have introduced previewEntries as an intended replacement for config. Since config doesn't actually mean anything, and previewEntries does. Since we're in a breaking release, I'd propose that we take the opportunity to sort this out properly, even if it means introducing a new preset (previewAnnotations?)

WDYT?

@tmeasday
Copy link
Member Author

tmeasday commented Sep 11, 2022

What's the difference between previewEntries and config? Is it that config exports are treated as annotations (e.g. parameters etc.)?

Yes that's the difference.

I think @ndelangen and I may have introduced previewEntries as an intended replacement for config.

I don't believe so, but @ndelangen could say for sure.

This is where they are pulled in by the webpack builder (this is definitely a set of "entries" in the preview, not "configs"/annotations):

result = result.concat(await options.presets.apply('previewEntries', [], options));

Actually I had it wrong, previewEntries is used in on place by our code:

export const previewEntries = (entries: any[] = []) => {
entries.push(require.resolve('@storybook/core-client/dist/esm/globals/globals'));
return entries;
};

I would suggest we replace it with a config (or, as you say a previewAnnotations) and not bother maintaining two subtly difference concepts (just make sure we don't export if we don't want to provide annotations). WDYT @ndelangen ?

@tmeasday
Copy link
Member Author

tmeasday commented Sep 12, 2022

  • Replace above previewEntries with a config
  • Rename config to previewAnnotations
  • Deprecate config + warn in common-preset.ts
  • Add migration notes

@tmeasday
Copy link
Member Author

tmeasday commented Sep 12, 2022

Note previewAnnotations was added in #17755 @shilman, however the builder code all still uses config to access it. I reversed that and added a warning if config is set.

image

Switch `config` and `previewAnnotations`, and warn on the former.
@tmeasday
Copy link
Member Author

Add to migration notes

@tmeasday tmeasday force-pushed the tom/sb-740-remove-previewentries-from-vite-builder branch from c9d13c5 to 62cabad Compare September 13, 2022 05:17
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking good!

@shilman shilman changed the title No longer handle previewEntries in the vite builder. Presets: Replace config with previewAnnotations, remove previewEntries Sep 13, 2022
@tmeasday tmeasday merged commit 52cd4f4 into next Sep 13, 2022
@tmeasday tmeasday deleted the tom/sb-740-remove-previewentries-from-vite-builder branch September 13, 2022 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-vite maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants