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

Core: Fix sorting by __namedExportsOrder #16626

Merged
merged 2 commits into from Nov 8, 2021
Merged

Conversation

shilman
Copy link
Member

@shilman shilman commented Nov 7, 2021

Issue: #15574

What I did

Story sorting has changed completely in 6.4, and the old code that handled __namedExportsOrder is broken. I'm not even sure if we need it anymore @tmeasday

Instead:

  • Fix v6 handling in StoryStoreFacade
  • Fix v7 handling in the story index by updating CSFFile

How to test

See attached tests & updated examples

@nx-cloud
Copy link

nx-cloud bot commented Nov 7, 2021

Nx Cloud Report

CI ran the following commands for commit 0787dfc. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@shilman shilman requested a review from tmeasday November 7, 2021 16:17
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This LGTM but should we remove the code from processCSFFile which (uselessly) considered __namedExportsOrder? :

// prefer a user/loader provided `__namedExportsOrder` array if supplied
// we do this as es module exports are always ordered alphabetically
// see https://github.com/storybookjs/storybook/issues/9136
if (Array.isArray(__namedExportsOrder)) {
exports = {};
__namedExportsOrder.forEach((name) => {
if (namedExports[name]) {
exports[name] = namedExports[name];
}
});
}

@shilman
Copy link
Member Author

shilman commented Nov 8, 2021

@tmeasday yes but I wasn't sure what impact that would have. trying it now.

@shilman
Copy link
Member Author

shilman commented Nov 8, 2021

self-merging @tmeasday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants