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

[WIP] Plugin Ordering #5735

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[WIP] Plugin Ordering #5735

wants to merge 13 commits into from

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented May 15, 2017

Fixes #5623

Summary

TL;DR - basic topological ordering of plugins.

Currently this is just the minimal change necessary to make class properties and decorators work (not a final approach or anything and not done yet)

This PR adds 2 more properties to the Plugin object:

  • capabilities: an array of values (currently a string, maybe better as a number/semver like in package.json) to represent what a plugin can transform
  • dependencies: an array of values to represent which syntax needs to be transformed after this plugin is run (it depends on that syntax being there beforehand so it should go first)
  • before/after: an array of values to represent which plugin capabilities should be run before/after

This is technically not a breaking change since it's opt-in by the plugins but it would be easier done in 7.x?

TODO

  • can defer to separate discussion: how to specify dependencies and standardize on them (a string, package.json, semver)
    • maybe expose some symbols instead of a string? like babel.capabilities.arrowFunctions = Symbol()
  • handle cycles (dep A -> dep B -> dep C -> dep A) - just error with a good error message this probably isn't an issue and/or we will fix it when it's reported
  • do we need to handle presets? seems ok
  • later: handle multiple plugins providing the same capability - error with a good message because the plugin/preset is being included multiple times
  • add dependencies/capabilities for all internal plugins
  • how to handle multiple passes (deprecate passPerPreset and introduce a new option to enable it per preset individually?) - maybe not as important
  • how does this tie into preset-env (native environments provide the same capabilities and we need to use the same interface) https://github.com/babel/babel-preset-env/blob/master/data/plugin-features.js

Use case

  • plugin ordering of official plugins (both in spec and proposals)
  • third party plugins ordering
  • fork on outputting syntax based on capabilities
  • better config validation

cc @benjamn, @rwjblue

@hzoo hzoo added PR: New Feature 🚀 A type of pull request used for our changelog categories PR: WIP labels May 15, 2017
@mention-bot
Copy link

@hzoo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth, @Alxpy and @existentialism to be potential reviewers.

@hzoo hzoo requested a review from loganfsmyth May 15, 2017
@hzoo hzoo changed the title WIP plugin ordering [WIP] Plugin Ordering May 15, 2017
@hzoo hzoo requested a review from danez May 15, 2017
@hzoo hzoo requested a review from gaearon May 15, 2017
@@ -42,6 +42,8 @@ export default function ({ types: t }) {
return {
inherits: syntaxClassProperties,

capabilities: ["classProperties"],
Copy link
Member Author

Choose a reason for hiding this comment

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

Opt-in so 3rd party plugins can also provide capabilities/dependencies. Right now it's just a string but maybe it should be a versioned number?

@hzoo hzoo removed the request for review from gaearon May 15, 2017
@rwjblue
Copy link
Member

rwjblue commented May 16, 2017

I like it, thanks for working on this! As I reviewed the sortPlugins logic I thought you might be able to take advantage of dag-map which is what ember-cli uses for a similar purpose.

@hzoo
Copy link
Member Author

hzoo commented May 16, 2017

Thinking about it now, it's probably fine to not have every plugin have to have a capability anyway since it's opt-in and can be incremented added if someone needs it. It's easiest to do for proposals ironically.

@hzoo
Copy link
Member Author

hzoo commented May 17, 2017

Ok as discussed on slack, maybe dependencies isn't a good word for this because for this specific case. as @rwjblue describes:

  • plugin-a literally requires that plugin-b exists (currently what inherits already does)
  • plugin-a wants to know that it is ran before plugin-b if it happens to exist (kinda like an optionalDependency and specifying order)
  • ok maybe as a first pass "dependencies" are optional, we keep inherits


// Calculate the capabilities that each plugin provides, map to a single name.
// Multiple plugins with the same capability are not allowed as that can
// become a cyclic dependency.
Copy link
Member Author

@hzoo hzoo Jun 9, 2017

Choose a reason for hiding this comment

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

I don't think it will become a cyclic dependency - don't think there are any actual ones? Our primary goal is to fix plugin ordering in Babel itself (only syntax), then minifier, then 3rd party plugins (not as important).

It's just about telling the user if a plugin is included more than once just because it doesn't have to be.

Copy link
Member Author

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

Don't think it's controversial to land this as is:

  • removes passPerPreset (undocumented, buggy, shouldn't be a boolean flag for all presets anyway, not well known/used)
  • fix plugin ordering for class properties/decorators 👍
  • opt-in for other plugins while it's not required yet

@codecov
Copy link

codecov bot commented Jun 13, 2017

Codecov Report

Merging #5735 into 7.0 will increase coverage by 0.04%.
The diff coverage is 88.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5735      +/-   ##
==========================================
+ Coverage   85.09%   85.14%   +0.04%     
==========================================
  Files         284      284              
  Lines        9911     9942      +31     
  Branches     2768     2769       +1     
==========================================
+ Hits         8434     8465      +31     
+ Misses        976      974       -2     
- Partials      501      503       +2
Impacted Files Coverage Δ
...ges/babel-plugin-transform-decorators/src/index.js 94.28% <ø> (+0.95%) ⬆️
packages/babel-core/src/config/index.js 50% <ø> (ø) ⬆️
...bel-plugin-transform-class-properties/src/index.js 94.59% <ø> (ø) ⬆️
packages/babel-core/src/config/plugin.js 36.36% <33.33%> (-1.14%) ⬇️
packages/babel-core/src/config/option-manager.js 78.12% <92.15%> (+1.93%) ⬆️
...ckages/babel-core/src/transformation/file/index.js 87.25% <95%> (+0.05%) ⬆️
packages/babel-traverse/src/visitors.js 86.66% <0%> (+0.95%) ⬆️
packages/babel-traverse/src/path/modification.js 73.07% <0%> (+0.96%) ⬆️
packages/babel-traverse/src/path/context.js 87.06% <0%> (+1.72%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cc1cbf...3e27d17. Read the comment docs.

@hzoo hzoo mentioned this pull request Jun 13, 2017
9 tasks
plugins.forEach((pluginTuple, index) => {
const plugin = pluginTuple[0];
if (!plugin.key) { plugin.key = `${unnamedPluginPrefix}${index}`; }
});
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need this bit anymore, given that we're making name compulsory. i'll remove it in my naming PR

if (plugin.capabilities) {
plugin.capabilities.forEach((capability) => {
const existingPluginName = capabilitiesToPluginIdMap[capability];
if (existingPluginName !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

prob safer to just say if (existingPluginName), this way empty strings can also be caught. (although that shouldn't be allowed in the first place).

throw new Error("Plugin .after must be an array");
}
if (!Array.isArray(plugin.before)) {
throw new Error("Plugin .before must be an array");
Copy link
Contributor

Choose a reason for hiding this comment

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

Naive thinking perhaps, but judging by the conditionals above this one, it may freak out if plugin.before / plugin.after isn't present.

@@ -31,7 +34,8 @@ export default class Plugin {
this.pre = plugin.pre;
this.visitor = plugin.visitor;
this.capabilities = plugin.capabilities;
this.dependencies = plugin.dependencies;
this.after = plugin.after;
this.before = plugin.before;
Copy link
Contributor

Choose a reason for hiding this comment

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

based on my prev comment, might wanna do a this.x = plugin.x || []; to account for cases where its not present?

@sarupbanskota
Copy link
Contributor

sarupbanskota commented Jul 6, 2017

hi @hzoo, on #5911, I've synced work on this branch with 7.0. Went a little nuts trying to rebase, so I just did a merge. Would you pls review and merge it since I can't push directly to this branch?

Once that's done, I can get #5842 ready too - it's essentially good to go, but is failing because it depends on this branch which is currently failing.

cc @nathanhammond

@hzoo hzoo requested a review from jridgewell Jul 21, 2017
@danez danez closed this Aug 31, 2017
@danez danez reopened this Aug 31, 2017
@danez danez changed the base branch from 7.0 to master Aug 31, 2017
Copy link
Contributor

@hulkish hulkish left a comment

Choose a reason for hiding this comment

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

I feel like maybe a stronger approach here could be to introduce some kind of lifecycle hooks concept... to start with would be those known plugins which require one in particular to come before the other.

@sw-yx
Copy link

sw-yx commented Feb 5, 2019

hello, trying to catch up on this. What's blocking this today? is the TODO at the top up to date?

@JLHwung JLHwung removed the Has PR label Nov 14, 2019
@loganfsmyth loganfsmyth removed their request for review Feb 16, 2021
@danez danez removed their request for review Jul 7, 2022
@jridgewell jridgewell removed their request for review Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 A type of pull request used for our changelog categories PR: WIP Priority: High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Fix Plugin Ordering