Skip to content

Remove core-js dependency from @babel/register#9847

Merged
nicolo-ribaudo merged 1 commit intobabel:masterfrom
coreyfarrell:babel-register-no-core-js
Sep 6, 2019
Merged

Remove core-js dependency from @babel/register#9847
nicolo-ribaudo merged 1 commit intobabel:masterfrom
coreyfarrell:babel-register-no-core-js

Conversation

@coreyfarrell
Copy link
Copy Markdown
Contributor

Q                       A
Fixed Issues?
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Existing tests pass
Documentation PR Link
Any Dependency Changes? 👍
License MIT

The dependency core-js is not actually used by @babel/register so this PR removes it.

This module doesn't use core-js at all.
@babel-bot
Copy link
Copy Markdown
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10681/

@nicolo-ribaudo
Copy link
Copy Markdown
Member

While this could be removed, I am a bit worried that it will break users using @babel/register @babel/preset-env with corejs: 3 without explicitly listing it in their dependencies.

cc @babel/babel

Copy link
Copy Markdown
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

anyone relying on this being implicitly present already has a bug; this error will surface their broken package.json most efficiently, at build time.

@suchipi
Copy link
Copy Markdown
Member

suchipi commented Sep 3, 2019

Perhaps we should move the core-js dep to preset-env, then?

Copy link
Copy Markdown
Member

@chicoxyzzy chicoxyzzy left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Sep 4, 2019

Perhaps we should move the core-js dep to preset-env, then?

preset-env doesn't rely on core-js. Only it's output, when explicitly enabled by the user, does.
Also, we would need somehow to add both core-js 2 and 3 without aliasing them so it's not possible.

this error will surface their broken package.json most efficiently, at build time

Actually, @babel/runtime is executed at runtime by definition 😛
And also if it wasn't, the error would be a MODULE_NOT_FOUND thrown at runtime because of the injected core-js imports.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Sep 4, 2019

ok, fair point on that. still tho, theoretically any test of any kind on a babelified file would expose it?

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Sep 4, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit aa7678f into babel:master Sep 6, 2019
@coreyfarrell coreyfarrell deleted the babel-register-no-core-js branch September 13, 2019 17:37
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants