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

CLI: Remove babel-register in favor of esm #7823

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

ndelangen
Copy link
Member

Issue: having babel-register to transpile our CLI code when starting is bad

What I did

I migrated it to use -r esm instead

@ndelangen ndelangen added maintenance User-facing maintenance tasks cli labels Aug 20, 2019
@ndelangen ndelangen added this to the 5.2.0 milestone Aug 20, 2019
@ndelangen ndelangen self-assigned this Aug 20, 2019
@vercel
Copy link

vercel bot commented Aug 20, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@ndelangen ndelangen requested review from Hypnosphi and removed request for stijnkoopal August 20, 2019 13:58
@ndelangen
Copy link
Member Author

@Hypnosphi Could you submit your review for this possibly as soon as is convenient for you? We'd like to get this merged asap.

Thanks! ♥️

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.

❤️

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 20, 2019

What is the lowest Node version that our CLI officially supports?

@Hypnosphi
Copy link
Member

Before the change, it was 8: https://github.com/storybookjs/storybook/blob/next/lib/cli/.babelrc#L5

Maybe we should just transpile this package, as we do with other ones?

@shilman
Copy link
Member

shilman commented Aug 20, 2019

@Hypnosphi esm is compat with Node 6+ so i think this is a safe change.

@vercel vercel bot temporarily deployed to staging August 20, 2019 14:52 Inactive
@ndelangen ndelangen force-pushed the tech/stop-using-babel_register branch from dfdbd0d to 8fde590 Compare August 20, 2019 15:15
@Hypnosphi
Copy link
Member

@shilman but our own sources can use some newer features

@ndelangen
Copy link
Member Author

@Hypnosphi We currently don't AFAIK,

I'm ok with someone changing it over to transpile everything instead.

@shilman
Copy link
Member

shilman commented Aug 20, 2019

@Hypnosphi open to doing that in a subsequent PR but want to get this out to address a core-js related issue in the CLI

@shilman shilman changed the title REMOVE need for babel-register in our CLI CLI: Remove babel-register in favor of esm Aug 20, 2019
@shilman shilman merged commit bd2aa03 into next Aug 20, 2019
@shilman shilman deleted the tech/stop-using-babel_register branch August 20, 2019 15:58
@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 24, 2019

is bad

Some additional context wouldn't hurt our future selves

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

Successfully merging this pull request may close these issues.

3 participants