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

Jump to bottom

FIX error of load order when using configure in preview|config.js #10159

Merged
merged 12 commits into from
Mar 20, 2020
Merged

FIX error of load order when using configure in preview|config.js #10159

merged 12 commits into from
Mar 20, 2020

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Mar 19, 2020 •
edited
Loading

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Mar 19, 2020
@ndelangen ndelangen added this to the 6.0.0 milestone Mar 19, 2020
@ndelangen ndelangen requested a review from tmeasday March 19, 2020 09:42
@ndelangen ndelangen self-assigned this Mar 19, 2020
@tmeasday
Copy link
Member

Can you explain the idea here a bit more?

@ndelangen
Copy link
Member Author

preview & config can call configure which should always happen AFTER all global decorators & args are set.

shilman
shilman reviewed Mar 20, 2020
View reviewed changes
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.

This is pretty gnarly

shilman
shilman reviewed Mar 20, 2020
View reviewed changes
lib/core/src/server/preview/entries.js
const isGenerated = /generated-(config|other)-entry/;
const isGeneratedConfig = /(?:preview|config)\..+-generated-config-entry/;

return stable(entries.slice(0), (a, b) => {
Copy link
Member

Choose a reason for hiding this comment

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

"preview & config can call configure which should always happen AFTER all global decorators & args are set." - better explanation

shilman
shilman approved these changes Mar 20, 2020
View reviewed changes
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.

i'm ok with this with better comment & cleanup the webpack. @tmeasday?

@tmeasday
Copy link
Member

Ok yeah it makes sense to me. Let’s make sure in the code the reason for it is clear

@ndelangen
Copy link
Member Author

Is this code comment acceptable?

@ndelangen ndelangen merged commit 1dfb84e into next Mar 20, 2020
@ndelangen ndelangen deleted the fix/generated-load-order branch March 20, 2020 10:53
tmeasday
tmeasday reviewed Mar 23, 2020
View reviewed changes
lib/core/src/server/preview/entries.test.js
'/Users/dev/Projects/GitHub/storybook/core/examples/web-components-kitchen-sink/.storybook/preview.js-generated-config-entry.js',
]);
});
it('should move stories-type all other generated entries', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is after missing from this sentence?

@tmeasday
Copy link
Member

I think we need a bit more explanation. Something like:

We need to ensure that all parameters and decorators that are added by preview entrypoints added by addons happen before any configure() calls executed by the user's preview.js (or config.js), or by main.js:stories. As those addons will create generated entries, this means we need to ensure all generated entries come before all other entries (generated or otherwise).

ndelangen added a commit that referenced this pull request Mar 23, 2020
@ndelangen
IMPROVE wording of test & code-comment
2ea4cb5 
#10159 (comment)
@ndelangen
Copy link
Member Author

fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmeasday tmeasday tmeasday left review comments

@shilman shilman shilman approved these changes

@alterx alterx Awaiting requested review from alterx

@igor-dv igor-dv Awaiting requested review from igor-dv

Assignees

@ndelangen ndelangen

Labels
maintenance User-facing maintenance tasks
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ndelangen @tmeasday @shilman