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: Replace preset-env polyfills with babel-polyfills #13055

Merged
merged 15 commits into from
Dec 1, 2020

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Nov 9, 2020

Issue: #11255.

Another attempt, this time it depends on babel/babel-polyfills#40

presumably what they will want us to do is use the babel-polyfills plugin rather than the preset-env corejs config. the docs seem to suggest thats the way forward, so this is just a draft of doing it that way

@43081j 43081j marked this pull request as draft November 9, 2020 11:20
@nicolo-ribaudo
Copy link

presumably what they will want us to do is use the babel-polyfills plugin rather than the preset-env corejs config

Yes, this will be the recommended way starting from Babel 8 👍

I want to add that even if this package's version is currently 0.0.6, it passes all the core-js tests we have in @babel/preset-env. The only reason for a 0.0.x version is that we'll release it as stable and officially make everyone migrate only once we release Babel 8.

The way you migrate to the new plugin in this PR is correct.

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

awesome, thanks for taking time to explain that.

and from what i can gather, setting the new flag to true means the paths are resolved from the cwd?

@nicolo-ribaudo
Copy link

and from what i can gather, setting the new flag to true means the paths are resolved from the cwd?

It is relative to the config file where storybook's preset is enabled, or to cwd if the preset is enabled in the programmatic options.

I don't know how you enable this preset, but maybe it would be better to:

  • Add core-js as a dependency to Storybook
  • Use absoluteImports: require.resolve("core-js"), so that it always resolves Storybook's core-js dependency.

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

yeah thats a good idea i think.

we already have core-js as a dependency but should probably resolve to it here rather than leave it up to babel. ill change it

@merceyz
Copy link
Contributor

merceyz commented Nov 9, 2020

Should be fine to remove core-js as a dependency in the e2e tests now
ref

// TODO: remove when https://github.com/storybookjs/storybook/issues/11255 is solved
'core-js',

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

done.

also i noticed almost every package we have has a core-js dependency. are they still needed?

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

@nicolo-ribaudo do we have any timeline for when the merged pr in babel-polyfills will be published? just so i know to keep an eye on it if its soon. or if you could let me know in here when it is, that'd be super useful

@nicolo-ribaudo
Copy link

I'm updating some deps and then I'll release.

@nicolo-ribaudo
Copy link

Released

@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

great, thanks for the quick turnaround on that :D

have pushed the updated one here too

package.json Outdated Show resolved Hide resolved
@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

i have tried this in two test repos:

  • one where core-js@2 is a top level dependency, currently broken with storybook master
  • one where core-js@3 is a top level dependency, currently works with master

with this branch, both worked fine! maybe we have fixed it finally? 😂

@43081j 43081j marked this pull request as ready for review November 9, 2020 15:23
@43081j 43081j requested a review from ndelangen as a code owner November 9, 2020 15:23
@43081j
Copy link
Contributor Author

43081j commented Nov 9, 2020

i do still need to know if all the sub-packages can drop the corejs dependency too.

pretty much every package we have has a corejs dependency in their package.json (every app/, addon/, etc).

@merceyz
Copy link
Contributor

merceyz commented Nov 9, 2020

also i noticed almost every package we have has a core-js dependency. are they still needed?

Depends if they actually use it or if it was a workaround to get it to work before this PR

scripts/run-e2e-config.ts Outdated Show resolved Hide resolved
@merceyz
Copy link
Contributor

merceyz commented Nov 27, 2020

The packages that are transpiled before publish will need to declare core-js as a dependency still, so that probably needs to be reverted

@ndelangen
Copy link
Member

@shilman let's merge?

Copy link
Contributor

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

PnP is happy so LGTM 👍

@shilman
Copy link
Member

shilman commented Nov 27, 2020

@ndelangen I'm going to make the cut over to 6.2 on Monday. Will merge then.

Copy link
Contributor

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Need to remove useBuiltIns: 'usage', corejs: '3' from here as well

{ shippedProposals: true, useBuiltIns: 'usage', corejs: '3' },

# Conflicts:
#	app/react/src/server/framework-preset-react.test.ts
@shilman shilman changed the title replace preset-env polyfills with babel-polyfills Core: Replace preset-env polyfills with babel-polyfills Dec 1, 2020
@shilman shilman merged commit 9453155 into storybookjs:next Dec 1, 2020
@shilman shilman added the core label Dec 1, 2020
@43081j 43081j deleted the corejs-second-attempt branch December 1, 2020 07:49
@ndelangen
Copy link
Member

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants