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

resolve core-js to storybook's version #12312

Closed
wants to merge 1 commit into from

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Aug 29, 2020

This is just a draft to show where the problem lies.

Fixes #11255.

What is the issue?

Basically, somewhere down the line we use core-js inside storybook (presumably to polyfill everything as a catch-all?).

When we bundle the JS to be executed in storybook's iframe, we take the user's sources and our own so we can effectively join up storybook's utils/controls/etc with the user's stories.

The problem begins if the user has their own core-js installed (not necessarily as a direct dependency).

Take the following imaginary dependency structure:

- some-dependency
-- core-js@2
- @storybook/html
-- core-js@3

assume this resulted in the following directory structure:

node_modules
- /core-js (2.x)
- some-dependency
-- /core-js (DEDUPED, 2.x)
- /@storybook
-- /html
--- /core-js (3.x)

This now means the core-js at top level is 2.x NOT 3.x, which is not the one we want.

Now, when we create our bundle, we will execute webpack in the USER's directory, not webpack's directory. This means the core-js we resolve will be the top-level one: 2.x, the wrong version.

Possible Solution

From looking at the code, i can see you've in fact encountered this before but from a different point of view: resolving storybook's own modules from outside storybook's directory.

So here i've just done the exact same thing, i have forcefully aliased core-js to the one webpack installs.

Problems / Questions

  • If someone genuinely depends on another corejs in their own code, that will probably be broken instead now as we will be forcing it to resolve a version different to what is in their package
  • Where on earth is core-js being pulled in? for my own sanity this would be nice to learn, i have absolutely no idea because of how dynamic all this stuff is. its all good us 'fixing' it, but we really should understand why we have the problem in the first place (why we have core-js)
  • it feels like we're fixing one part of a wider issue... who knows what other dependencies we have that could clash in the same way, literally any dependency we pull in will probably have this issue

cc @ndelangen @shilman

@43081j
Copy link
Contributor Author

43081j commented Sep 2, 2020

Just an update that norbert and i discussed this and it may not be so simple as the solution here, so ill have a look through it again this weekend.

This fix (in this pr) would work for most people but could also cause weirdness for those of you who use an older corejs (as we would be effectively forcing it to resolve to the newer one, which could break your code).

@merceyz
Copy link
Contributor

merceyz commented Sep 4, 2020

@JLHwung would babel be open to a absoluteRuntime like setting for @babel/preset-env?

@43081j
Copy link
Contributor Author

43081j commented Sep 5, 2020

IMO the 'fix' does belong in babel, in that we should be allowed to specify the exact core-js path if we need to.

https://github.com/babel/babel/blob/20fd450ab7855622674f56772879953b49c95f23/packages/babel-preset-env/src/utils.js#L63-L65

This seems to be where it gets mapped. If it allowed an optional explicit path, our problem would likely be solved I imagine (just force it to map to storybook's core-js).

@merceyz
Copy link
Contributor

merceyz commented Sep 5, 2020

Agreed, the same should be possible for regenerator-runtime as well. @babel/plugin-transform-runtime already has this but only for @babel/runtime imports

@43081j
Copy link
Contributor Author

43081j commented Sep 26, 2020

To avoid this going stale I'll catch up on it this weekend and see if there's any chance I can contribute a fix in Babel's end of things.

Though a work around would be nice too. I'll have another try

@merceyz
Copy link
Contributor

merceyz commented Sep 26, 2020

I've worked around this in the past by adding a babel plugin that changes the core-js imports it comes across to absolute imports, this makes the assumption that the user doesn't import a different version of core-js manually in any of the files we transpile though

@43081j
Copy link
Contributor Author

43081j commented Sep 26, 2020

yup which unfortunately isn't an adequate solution here. we need to support mixed core-js versions while simultaneously changing only our paths to be absolute.

there will be a solution even if that means fixing babel.

given the fact they haven't even managed to respond to our issue yet, though, a fix (even if we contribute it) doesn't sound like something that would land in a release any time soon. so i'll see if i can find a workaround that doesn't need changes in babel meanwhile.

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

Closing this as i think #13055 should be a better fix.

@43081j 43081j closed this Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storybook@6 throws a core-js error
2 participants