[RFC] Pass per preset

[DmitrySoshnikov]

Rationale

See discussion and the issue description in #6730: Plugins order is not respected.

This introduces "pass per preset" feature, spawting a new traversal for each preset in case if the passPerPreset is true (default is false).

This gives opportunity to define "before" and "after" presets, mimicking a similar feature from Babel 5. A rationally for this is to make plugins as short as possible, and handle only needed nodes, not dealing with "unrelated" node in order to avoid potential collisions in case if presets are merged.

RFC: design notes

Currently I handle only plugins in presets. Are there actual practical use case when a preset may define its own options, other than plugins? The options per preset are inherited from main options, overriding needed fields, such as plugins.

Test plan

make test. Will add more tests "per preset" after main review.

[codecov-io]

Current coverage is 85.05%

Merging #3281 into master will decrease coverage by -0.25% as of 845a4fa

@@            master   #3281   diff @@
======================================
  Files          215     215       
  Stmts        15751   15731    -20
  Branches      3373    3363    -10
  Methods          0       0       
======================================
- Hit          13436   13380    -56
- Partial        679     712    +33
- Missed        1636    1639     +3

Review entire Coverage Diff as of 845a4fa

Powered by Codecov. Updated on successful CI builds.

[sebmck]

We could probably get away with enabling this by default in Babel 6. Otherwise we could just leave this disabled and enable it in Babel 7.

[amasad]

you can add hidden: true so it doesn't show up on the website I think. cc @thejameskyle

[jamiebuilds]

I don't know what you mean by showing up on the website?

[DmitrySoshnikov]

@thejameskyle, I think these options are meant: http://babeljs.io/docs/usage/options/

[sebmck]

This looks awesome @DmitrySoshnikov! Code looks good although I'm not sure how this impacts the other non-plugin options. A preset could specify an option like env.development.plugins and I'm not sure how we handle the normalisation in this case.

I'll leave this open for a day or two to get feedback from @amasad, @hzoo, @loganfsmyth, @jmm etc if they have any thoughts.

[loganfsmyth]

Having an external flag that controls this seems really weird to me. I could see flagging a preset itself as wanting its own pass. What is the motivation for making this now something that users need to know about? Babel's config is already extremely hard for general users to figure out, adding more flags is something I find scary.

If our goal is to control ordering for presets, I think we'd be better off by making that an explicit preset-level config flag.

[DmitrySoshnikov]

Yep, totally agree that it might be an unnecessary option to be controlled by users, and more is an implementation detail. The only reason I made it yet via the flag, is to preserve the existing state, not causing potential breakages, and once we'll have fully tested in production that things work fine with "pass per preset", we can delete the flag. But yeah, I don't wanna make it a user option (maybe we can make it an "internal flag" somehow for now?). Or, we can go without the flag right away, I'm fine with this either. Let me guys know.

Re: preset options. If preset options are just nested sub-options, and practically may have anything that may appear at top-level, then we should handle all of them of course, not only plugins (the same what mergeOptions does now, but for each recursive sub-level). In this case a preset may always inherit a top-level chain. Can it be the case like:

{
  plugins: ['a', 'b', 'c'],
  presets: [
    'foo-bar',
    {
      plugins: ['d', 'e'],
      presets: [
        {
          plugins: ['f', 'g'],
          env: {
            development: {
              plugins: ['h', 'i']  
            }
          }
        }
      ]
    }
  ]
}

This arbitrary recursive nested sub-structures seems a bit harder to maintain, and to deal with for users. Probably we can stick with come practical use-cases for now, e.g. allow in presets only limited sub-set of possible options, and don't allows presets within presets, within presets. Unless again there is a practical implication to support it, in which case we can handle it.

[hzoo]

Re: Preset options/more global options could be useful but I think @kittens didn't want to make presets that complicated?
https://phabricator.babeljs.io/T2756 - An example though would be specifying loose mode for all plugins instead of doing it for each one.

[hzoo]

Sounds like @loganfsmyth's suggestion could be better in the long term? If we want to be safe we should leave it off by default?

[amasad]

Let's make sure it's not a public option. We can land this and start dogfooding this our self and then enable it by default in Babel 7.

[amasad]

can you add an _ to the name so that we make sure it's a private option?

[amasad]

Other than that, code looks good to me.

[DmitrySoshnikov]

@kittens: if we want to handle other options in presets (except plugins), then we could call mergeOptions here recursively, passing options "to merge into" as a parameter. So it'll be preset options, but not just top-level this.options, when it calls merge(this.options, opts); below.

It may look a bit weird though as mergeOptions(preset, preset, ...);. Or we can refactor mergeOptions since currently it does much more than actual merging. It does also normalization, e.g. preset.plugins = OptionManager.normalisePlugins(...) -- that exactly what we need. We don't wanna merge preset into itself, but we want to merge e.g. nested stuff in the presets like envOpts if they appear in the preset.

Or, better, to limit presets only to one nested level, and handle only small sub-set of options 😄 Let me know.

[DmitrySoshnikov]

Ok, I did some manual testing of the API with passPerPreset as true, and false, and confirm that it works as expected. With true our plugin has access to the NumberTypeAnnotation type alias. With false, that node is null and throws, since Flow already removed it.

var code = babel.transform('type Foo = number; let x = (y): Foo => y;', {
  passPerPreset: true,
  presets: [
    // First preset with our plugin, "before"
    {
      plugins: [
        new Plugin({
          visitor: {
            Function(path) {
              var node = path.node;
              var scope = path.scope;

              var alias = scope
                .getProgramParent()
                .getBinding(node.returnType.typeAnnotation.id.name)
                .path
                .node;

              console.log(alias.right.type); // NumberTypeAnnotation
            }
          }
        })
      ]
    },
    // ES2015 preset
    require('babel-preset-es2015'),
    // Third preset for Flow.
    {
      plugins: [
        require('babel-plugin-syntax-flow'),
        require('babel-plugin-transform-flow-strip-types')
      ]
    }
  ]
}).code;

console.log(code);

// "use strict";
// 
// var x = function x(y) {
//   return y;
// };

[hzoo]

Awesome stuff @DmitrySoshnikov! Would be cool if we can turn your example into a test though

[hzoo]

Docs: babel/website#710, babel/website#711

[jmm]

I wasn't following this too closely, but I had the same thought as @loganfsmyth:

Having an external flag that controls this seems really weird to me. I could see flagging a preset itself as wanting its own pass.

And I thought people agreed with that. E.g.:

@hzoo:

Sounds like @loganfsmyth's suggestion could be better in the long term?

@amasad:

Let's make sure it's not a public option.

But now we're documenting it?

https://github.com/babel/babel.github.io/pull/710/files

https://github.com/babel/babel.github.io/pull/711/files

EDIT: oh...and the release notes

https://github.com/babel/babel/releases/tag/v6.5.0

And people are already talking about it in other projects:

facebook/relay#714

[hzoo]

I put in the notes

Depending on usage, we will switch the way this is used to instead define a explicit preset-level config flag (rather than the current global one).

If we don't want it there, we can remove it from the docs (just revert those PRs)? Do you think we should clarify it further this is a experimental feature that may change to the above?

[jmm]

I was under the impression it wasn't going to be documented as public API at all for the time being.

Do you think we should clarify it further this is a experimental feature that may change to the above?

At this point we could make an additive change, but couldn't change it in a way that breaks BC except in a major.

[hzoo]

I thought it would be hard to know how it would be used (unless we are just testing it ourselves). I mean either way it's released as an option so people would be using it right?

[jmm]

I could have misunderstood, but that's basically what I thought the plan was -- make it available for people involved in these discussions to try it out without committing to it as public API yet. E.g.:

Let's make sure it's not a public option. We can land this and start dogfooding this our self and then enable it by default in Babel 7.

And if the intent was for it to be public, what was the purpose of this change?:

--- a/packages/babel-core/src/transformation/file/options/config.js
+++ b/packages/babel-core/src/transformation/file/options/config.js
@@ -185,9 +185,10 @@ module.exports = {
     type: "string"
   },

-  passPerPresset: {
+  passPerPreset: {
     description: "Whether to spawn a traversal pass per a preset. By default all presets are merged.",
     type: "boolean",
-    default: false
+    default: false,
+    hidden: true,
   },
 };

[hzoo]

Yeah I messed up here then - I just was documenting stuff in the changelog so I figured we could put it in and I forgot we were planning on doing this. No one noticed so I guess it's public now 😅 anyway.

[jmm]

In case there are situations like this in the future maybe we should come up with a better way to indicate this kind of thing. Would a label work for that do you think? I know doing the excellent changelongs is a lot of work (thank you! 😃) and it must be easy to miss / forget about this kind of thing that's just indicated in the comments, from weeks ago at that.

[hzoo]

Yeah for sure - I think part of it was that we waited a while for this. Yeah a label for this would be good idea - what would be a good label? just like private api or something?

[jmm]

Yeah, I'd say just something straightforward that would be likely to get the attention of someone merging / adding docs / doing a release and tip them off that it shouldn't be documented / mentioned in the release notes, etc. private api seems reasonable. Since you've been doing most of the work on the changelogs, I'd suggest that you choose a name, and if we decide something else is better later it shouldn't be too big of a deal to switch I'm guessing.

If necessary we can also start adding descriptions of the labels somewhere -- in doc/, a wiki, etc.

[hzoo]

Ok https://github.com/babel/babel/wiki/PR-Labels. I cleaned up all the old issue labels - and maybe we don't need some of the other ones either.

[sebmck]

We should just backtrack and hide it and discourage it's usage. It's just going to add noise to the conversation about how we make plugins work well together in the same traversal, I don't think this is a good solution but it's a temporary one to unblock specific use-cases that we don't cater to very well.

[DmitrySoshnikov]

Without this option the plugins work is unpredictable. I'd say it should be documented (just for now), and then make a default behavior: "each preset in its own pass". The issue was raised by several use-cases already -- isn't it enough to make it clear that current design is kinda broken?

@kittens, it can be a temporary solution for sure, as long as there is a guarantee that the needed information is still preserved (the "lossy" thingy #3335 (comment) doesn't seems solves the issue fully, and there is a chance to end up with even more complicated and frustrating documentation).

That's said, I think we can just make "pass per preset" a default behavior, don't document it to avoid "noise to the conversation", and things "will just work".

[DmitrySoshnikov]

It could be just a note: "NOTE: at top-level or within one preset, all plugins are merged and ran in one traversal pass. However, each preset runs in its own pass.". Something like this, and that's it. (I'm sure you guys can phrase it even more clearly). This will avoid mentioning the Babel's issue, and mentioning this flag.

[ericclemmons]

@DmitrySoshnikov Just wanted to say thanks for this PR.

I've been experimenting with doing a sequential pass with babel so that I can convert stateless, functional React components into React.Component classes so that I can use my custom react-transforms (e.g. HMR, Catch Errors, etc.):

ericclemmons/babel-plugin-react-pure-to-class#1 [Work in Progress!]

(I'm ignorant to the rationale, but I originally assumed the orders would be defined by nested arrays, similar to Cerebral Signals, but that format is already widely used for passing options to plugins.)

I know this feature & syntax will be changing, but I'm glad this PR is out in the wild for testing :)

[ForbesLindesay]

Suggestion if this situation comes up in the future: passPerPreset_IF_YOU_USE_THIS_YOUR_CODE_WILL_BREAK_IN_MINOR_VERSIONS. That way it's impossible to use the feature without realising it's risky.

As for this being temporary, the lossy (and similar) solutions will only solve the problem of plugins that need to be strictly before core babel transforms for core ES201[5-6] features. What about other plugin ordering issues (e.g. typecheck then syntax-flow or flow-strip-types). I think there will be a lot of things that need to be forced into separate phases.

What I'm not sure is whether preset should be the only form of separation, or whether plugins should be able to somehow indicate that they belong to some set of plugins that require strict ordering

[jmm]

@ForbesLindesay

Suggestion if this situation comes up in the future: passPerPreset_IF_YOU_USE_THIS_YOUR_CODE_WILL_BREAK_IN_MINOR_VERSIONS. That way it's impossible to use the feature without realising it's risky.

Thanks, that's probably a good idea.

[fab1an]

I have two thoughts about this:

1. How about adding constraints instead of pass per presets, meaning that the user could explicitly state their intention to fix "bugs":

   {
       "presets": ["es2015"],
       "ordering": {
            "plugin-a": {
                  "before": "plugin-b"
            }
       }
   }

2. Another idea would to add transformation history and let plugins defer their execution.

   visitor: {
       CallExpression: function CallExpression(path, state) {
           if (state.history.indexOf("babel.plugin.x") >= 0) {
               return "RUN ME LATER";
           }
       }
   }

   This history could also be used to just output warnings You have run plugin-X before, this plugin won't work properly, please see <url>.

I'm in the situation that I want to publish 2 babel-plugins, where the A must run after B, if B is used as well. I'm planning on doing this by setting custom values in state, which seems very hacky.

[ephys]

@fab1an It would also easily solve the problem most users have with transform-class-properties, I'm not sure why what you propose isn't a thing already.

[fab1an]

@ephys @everyone I would like to have an answer to this as well.

I don't think there's a good generic answer, so just let people specify the edge-cases explicitely. I opened a new issue for this.