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

Jump to bottom

Instantiate presets before plugins #11689

Merged
merged 4 commits into from Oct 9, 2020
Merged

Instantiate presets before plugins #11689

merged 4 commits into from Oct 9, 2020

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 7, 2020 •
edited by gitpod-io bot

Q                       A
Tests Added + Pass? Yes
Documentation PR Link
License MIT

This is only needed for #12151, but I'm sending them as two separate PR to make it easier to review them one by one (since the diffs overlap, but are conceptually separate).

The e2e failure makes me think that I got plugins ordering wrong, I'll check.

@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: core area: config labels Jun 7, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 7, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit eab3c06:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 7, 2020 •
edited

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

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jun 7, 2020
Merged
@nicolo-ribaudo nicolo-ribaudo force-pushed the instantiate-presets-before-plugins branch from 790d0b8 to 17ebbf4 Compare June 7, 2020 23:07
@nicolo-ribaudo nicolo-ribaudo force-pushed the instantiate-presets-before-plugins branch from 17ebbf4 to a65b757 Compare October 7, 2020 14:15
@nicolo-ribaudo nicolo-ribaudo changed the base branch from master to main October 7, 2020 14:15
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Oct 7, 2020
Closed
4 tasks
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review October 7, 2020 16:18
@nicolo-ribaudo
Copy link
Member Author

Wow, preset ordering is hard. They sometimes run bottom to top, and sometimes top to bottom 🙃
In the old code this wasn't explicit, but was handled due to a specific order of push/unshift calls. I have now added an if to switch between push and unshift because I couldn't figure out how to properly reflect the old behavior.

@nicolo-ribaudo nicolo-ribaudo force-pushed the instantiate-presets-before-plugins branch from d8cc7ec to 3285782 Compare October 7, 2020 18:28
nicolo-ribaudo
nicolo-ribaudo commented Oct 7, 2020
View reviewed changes
packages/babel-core/test/api.js
Comment on lines 424 to +427
"five;",
"six;",
"twentyone;",
"twentytwo;",
Copy link
Member Author

Choose a reason for hiding this comment

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

Before the last commit, this was

twentyone
twentytwo
five
six

And it was causing a failure in the CRA e2e test.

JLHwung
JLHwung reviewed Oct 7, 2020
View reviewed changes
packages/babel-core/src/config/full.js Outdated
@@ -199,12 +188,32 @@ export default gensync<[any], ResolvedConfig | null>(function* loadFullConfig(
.map(plugins => ({ plugins }));
opts.passPerPreset = opts.presets.length > 0;

if (context.filename?.includes("RuntimeErrorContaine")) {
console.log(initialPluginsDescriptors.map(p => p.key));
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔Is that for debug purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 🙃
I'll remove it tomorrow

JLHwung
JLHwung approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

LGTM other than a nit.

@nicolo-ribaudo
Copy link
Member Author

I'm marking this as "do not merge" until I finish the other PR, so that I'm sure that what I did here is only and everything I need.

This was referenced Oct 9, 2020
Merged
Merged
@nicolo-ribaudo nicolo-ribaudo force-pushed the instantiate-presets-before-plugins branch from 48b104e to eab3c06 Compare October 9, 2020 14:44
@nicolo-ribaudo nicolo-ribaudo changed the base branch from main to feat-7.12.0/config-loading-updates October 9, 2020 14:44
@nicolo-ribaudo
Copy link
Member Author

Merging into feat-7.12.0/config-loading-updates

@nicolo-ribaudo nicolo-ribaudo merged commit 9770811 into babel:feat-7.12.0/config-loading-updates Oct 9, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the instantiate-presets-before-plugins branch October 9, 2020 16:07
nicolo-ribaudo added a commit that referenced this pull request Oct 14, 2020
@nicolo-ribaudo
Instantiate presets before plugins (#11689)
1b90d90
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Oct 19, 2020
Draft
4 tasks
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

Assignees
No one assigned
Labels
area: config outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicolo-ribaudo @babel-bot @existentialism @JLHwung