-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
fix(storybook-builder): import both globals and globalsNameReferenceM… #2620
fix(storybook-builder): import both globals and globalsNameReferenceM… #2620
Conversation
🦋 Changeset detectedLatest commit: 010eed6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -1,7 +1,8 @@ | |||
import rollupPluginNodeResolve from '@rollup/plugin-node-resolve'; | |||
import { getBuilderOptions } from '@storybook/core-common'; | |||
import { logger } from '@storybook/node-logger'; | |||
import { globals } from '@storybook/preview/globals'; | |||
// @ts-ignore | |||
import { globals, globalsNameReferenceMap } from '@storybook/preview/globals'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldnt just the globalsNameReferenceMap
be enough? Why are we using both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for retrocompatibility. Those using 7.5.3 or lower will have an error if we don't support the previous behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Could you add a comment to the import that we import both for this reason? Just so we'll know why this is in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :) just running CI to make sure everything passes, then we can merge it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, fixed linting problems
7561aaa
to
201887e
Compare
…ap from @storybook/preview/globals
201887e
to
010eed6
Compare
…ap from @storybook/preview/globals
What I did