Closed Domiii closed 2 years ago
Do you a have a reproduction repo you can share? If not, can you create one? See how to create a repro. Thank you! 🙏
Maybe a better question is what's breaking for you?
Ok, I see what's going on. There's actually a FIXME in the code, referenced above, which is that this logic should only be applied to *.stories.*
files. I guess the behavior you're seeing is a consequence of that FIXME not being implemented, so I can try to take a look at that this week.
That said, the pattern you're using in #17588 isn't officially supported, so I'm not sure whether it's a good long-term approach, i.e. it will break in 7.0's default configuration (but will be supported in a legacy mode)
hi @shilman,
I looked back at your comment and you mentioned that my pattern isn't officially supported. What is it that is not officially supported, if I may ask?
export * from
part? (which should not be different from manually exporting each one of those (see MDN → export
→ Re-exporting / Aggregating)).stories
pattern does not include the individual files (in this particular case), but only index.js
, and thus it does not know to pre-process the nested story files?
export default
part for all those files?As a reminder, I have an export default
with title
, component
etc., which seems to follow exactly the instructions in the docs:
// index.js
import Modal from '../components/Modal';
export default {
title: 'common/Modal',
component: Modal,
argTypes: {
// backgroundColor: { control: 'color' },
},
};
export * from './ConfirmBasic';
export * from './ConfirmMulti';
export * from './FormBasic';
export * from './Blocking';
Followed by the actual stories, which are defined, just like the docs say, e.g.:
// ConfirmBasic.js
// ...
export const ConfirmBasic1 = ModalTemplate.bind({});
ConfirmBasic1.args = { ... }
export const ConfirmBasic2 = ModalTemplate.bind({});
ConfirmBasic2.args = { ... }
// ...
It's the export *
part.
It's valid JS but we don't support that in CSF currently. To explain why, let me dive into some details about Storybook's default 7.0 behavior.
One of the biggest requests for Storybook is to improve its boot-up behavior. Storybook's boot-up time is largely a function of webpack, the default builder that we use. The only way we can make things dramatically faster is if we build less. So in 6.4 we introduced the on-demand architecture and followed it up in 6.5 with lazy compilation.
How it works is that Storybook does a static analysis of your CSF files. That means we parse the contents of the file and analyze its default export and the named exports in the file. We use this to build an index of all the stories in your storybook. Critically, we don't execute the code in your story files.
This means that some valid JS is not actually valid CSF as far as Storybook is concerned. For example, if you try to use a template literal string in the default.title
property, we will throw an error.
As for the export *
pattern, figuring out the named exports would require that we load in the file that is being re-exported to figure out the named exports. This is not something we currently do. We might add it as a feature in the future, but for now it will break, so I'm just pointing that out.
The following workaround would probably work, since we wouldn't need to load ./ConfirmBasic
to see what its contents are. Yes, it's more work:
export { ConfirmBasic1, ConfirmBasic2 } from './ConfirmBasic';
Thanks for the info!
Would probably be a good idea to explicitly document such deviations from expectation?
I think, in order to reduce boiler-plate while not re-exporting, I'll remove index.js
and replace it with some sort of _stories.common.js
that has the export default
and will be re-exported by all story files. A lot less work and a lot less error prone than specifically naming every re-export.
@shilman Again, thanks for the help! The stories are now all showing up again. I closed #17588 as a result of this.
However, the current behavior of adding __namedExportsOrder
to every Module
and making it enumerable
still really screws any type of code that facilitates re-exporting or batch-importing from modules. Can you please make it non-enumerable?
Re-exporting or batch-importing from modules is still a common pattern that is used in all kinds of other scenarios, entirely unrelated to storybook (e.g. re-exporting sagas, routines, actions etc...). None of that works now because __namedExportsOrder
sneaks into the set of imports.
As a reminder: the original post has a test case that you can run on any of the latest alpha releases:
import * as x from './x';
const n = Object.keys(x).length;
console.assert(n === 2); // n should be the amount of exports in `./x`, but it also contains an unwanted extra entry for `__namedExportsOrder`
I am using a hackfix for any batched imports/re-exports for now:
/**
* hackfix: storybook bug
* @see https://github.com/storybookjs/storybook/issues/17587
*/
export default function hackfixedImports(x) {
const { __namedExportsOrder, ...imports } = x;
return imports;
}
@Domiii how do you make it non-enumerable?
@shilman I'm not sure. I'm not even sure how you are getting Babel or webpack to add it in the first place? I have not yet found the code that automatically adds it to all modules?
I mean right now it just inserts the code
export const __namedExportOrder = ['A', ...];
Using a Babel plugin. Is there some other code that I could add that would make it non-enumerable?
This is the plugin but I'm less interested in the implementation and more in the result
https://github.com/storybookjs/babel-plugin-named-exports-order
@shilman Thanks for linking the babel plugin. (For reference: link to the relevant code).
I would say a general rule of thumb is: instrumentation should never change the behavior of second party code that is not directly affected by or affecting your framework/library.
Do you need this for all modules, or only for stories?
main.js
's can be used for both: configuring babel
as well as configuring stories
, it would be rather easy to just feed the stories
back to the configuration of babel-plugin-named-exports-order
.FYI: you can access the plugin's custom config like so:
function isSkipped(state) {
return !!state.skipped;
}
return {
visitor: {
Program: {
enter(path, state) {
const { opts: cfg, filename } = state;
console.debug(`visiting ${path} in "${filename}" with cfg =`, cfg);
if (!cfg.shouldInstrument(filename)) {
/**
* hackfix for skip
* @see https://github.com/babel/babel/issues/11147#issuecomment-902714234
*/
state.skipped = 1;
return;
}
}
},
X: {
enter(path, state) {
if (isSkipped(state)) return;
// ...
}
}
};
I'm not sure who is supposed to consume those injected exports
, but I would guess, it is only one or two dedicated consumer functions, and not something that needs to be generally available as an export
. In that case, you don't want to modify the module exports
.
Instead, you can keep a global Map.<string, CustomStorybookStuff>
that maps the per-module metadata to the filepath.
Since you already have a set of stories with file path information available at run-time, annotating that data with more module-specific stuff should not to be too bothersome.
E.g. by changing your "injecting an extra export
" to something like this:
require('@storybook/runtime').registerModule('filepath', {
__namedExportOrder: [...]
});
(However, you have to be careful when injecting require
statements. One of the things you will most likely need to do is setting sourceType to 'unambiguous'
. As an alternative, you can register a storybook global that you can attach the registerModule
function to etc. )
This has broken our storybook, where stories use a mobx-state-tree
store and the store uses this import to load a set of generic models.
import * as markTypes from '@plugins/drawingTools/models/marks/index.js'
I can hack our code to remove the __namedExportsOrder
key that has been added to markTypes
but the storybook shouldn't be modifying that import really.
Is this still being applied to non-story files? I get the following when running storybook --smoke-test
with 6.5.3:
WARN preview: [
WARN {
WARN "moduleIdentifier": "/Users/jon/Developer/web/node_modules/babel-loader/lib/index.js??ruleSet[1].rules[0].use[0]!/Users/jon/Developer/web/webpack/assets/javascripts/lib/page_ready/index.ts",
WARN "moduleName": "./webpack/assets/javascripts/lib/page_ready/index.ts",
WARN "loc": "6:0-30",
WARN "message": "The requested module './page_events' contains conflicting star exports for the name '__namedExportsOrder' with the previous requested module './live_watch'",
WARN "moduleId": "./webpack/assets/javascripts/lib/page_ready/index.ts",
...
page_ready/index.ts just re-exports from a bunch of different files:
import onPageReady from "./page_events";
export default onPageReady;
export * from "./live_watch";
export * from "./page_events";
presumably the __namedExportsOrder from live_watch & page_events is colliding?
@jdelStrother fix coming in the next 30min
Jeepers creepers!! I released https://github.com/storybookjs/storybook/releases/tag/v6.5.4-alpha.0 containing PR #18284 that references this issue. Upgrade today to the @prerelease
NPM tag to try it out!
npx sb upgrade --prerelease
Closing this issue. Please re-open if you think there's still more to do.
@Domiii Might look into better long-term solution later. Or not, because the problem goes away entirely if you use storyStoreV7
and that will become the default very soon (and is already vastly superior to the "old way")
Jeepers creepers!! I released https://github.com/storybookjs/storybook/releases/tag/v6.5.4-alpha.0 containing PR #18284 that references this issue. Upgrade today to the
@prerelease
NPM tag to try it out!npx sb upgrade --prerelease
Closing this issue. Please re-open if you think there's still more to do.
@shilman thanks, looks good
Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.4 containing PR #18284 that references this issue. Upgrade today to the @latest
NPM tag to try it out!
npx sb upgrade
This is Still Happening for "@storybook/builder-webpack4": "^6.5.9", "@storybook/manager-webpack4": "^6.5.9",
@shilman Please Look into it.
Hello,
Back to the original topic of this issues ( CSF not supporting export * from
)
this happens when moving from 6.4.22
to latest 6.5.13
In our project we kind of followed an unspoken rule of naming the story file the same as the only export inside that file so the following was kind of possible for us
vscode regexp search for export \* from ('[\S]*\/([\s\S]*?).stories';)
vscode replace with export { $2 } from $1
Sharing in case other stumble upon this as I did while investigating the issue
Describe the bug
The following code should return a
Module
objectx
.x
'skeys
,values
etc. should only be that of the named exports from given file. However, due to a recent change,__namedExportsOrder
is now also an enumerable property of that object, and messes with any code that assumes defaultModule
behavior.To Reproduce
Given two files:
System Please paste the results of
npx sb@next info
here.Possible Solution