Order of application in non-macro transforms chain

[awto]

This fixes the problem described in #95

While I know the applied technique can be called controversial, but it doesn't contradict macros usage, where macros applications aren't merged. At the moment I cannot specify where to run the macro in babel plugins chain, while it is possible to do with plugins.

Here, for example, just printing plugin (pprint.macro.js):

module.exports = require('babel-plugin-macros').createMacro(function({state}) {
  console.log(require("@babel/generator").default(state.file.scope.path.node).code)
})

I run it with:

$ npx babel --plugins babel-plugin-macros --presets @babel/preset-env input.js

And inside the input.js just a simple async generator function:

import "./pprint.macro"

async function* fn1() {
  yield await hi
}

Since the plugin should run before the preset I would expect this macro to output the text unchanged. However, now it outputs result of another plugin from "preset-env" which converts async generators to generators. But the preset-env after also converts generators to plain functions with regenerator. So I've got a kind of half-baked AST.

This PR isolates babel-plugin-macros pass and thus I'd get what I expect exactly. Users can decide where to apply this plugin in other babel plugins chain and get predictable behavior.

[kentcdodds]

I think I'm ok with this change. Could you please add a test or two to reveal the bug so it never comes back and we get 100% coverage back. Thanks!

[awto]

sure, I need to figure out how to make it first though. Could you suggest any way to make babel-plugin-tester to use plugins chain instead of a single plugin? Maybe some babel API function to merge them should be applied or some merged preset?

[kentcdodds]

Create a new pluginTester block and in it you can specify babelOptions which can have the list of plugins to apply.

[awto]

regarding the coverage problem, this may be actually the same problem, as in the test before "babel-prese-env" ate macro variable binding, but after the order is more strict if (binding) condition always true (nothing eats the binding), so I don't know yet, how to restore 100% coverage now, or is it safe to remove that condition.

[awto]

ok, I removed that check, but I'm not sure if it is indeed useless

[codecov[bot]]

Codecov Report

Merging #96 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #96   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          80     80           
  Branches       19     18    -1     
=====================================
  Hits           80     80

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	:arrow_up:

---

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 9cc8af1...3920bc2. Read the comment docs.

[kentcdodds]

Ok, cool, I'm good with this change. One last question. Do you think there's a risk of this breaking people unexpectedly?

[awto]

I think little risk exists, yes, if some transform depends on this ad-hoc order, but considering even the current test suite exposes a problem with swallowed binding, I believe it is worth applying. I also must admit I'm not an expert in babel API, I use my own framework instead of babel-traverse. But fixing this sounds like moving to a better state for me.

[kentcdodds]

Ok, thanks @awto. I'll go ahead and merge this. Hopefully it doesn't produce noticeable issues for anyone. If it does I'll revert immediately and we can evaluate things.

[awto]

Thanks.

Actutally, taking an opportunity to advertise my framework as an alternative to babel-traverse, such ordering problem, many problems related to AST mutation, simply don't exist there. It replaces Visitors with plain JavaScript generators. They are very easy to compose, unlike Visitors composition - this is just plain function composition.

Here is the framework @effectful/transducers and the whole big compiler done on top of it EffectfulJS. This first version was done on Visitors and quickly became boring. The transducer's version is still easy to extend, for now it has ~100 of pretty isolated options.

[mAAdhaTTah]

So I have an example where this breaks us: We're using CRA (which comes with macro support) with TypeScript. We're using the styled-components macro along with it, and previously, the TypeScript preset would run first and remove any type imports, then the macro would run as expected. With this version, the macro runs first and is erroring with the below:

Creating an optimized production build...
Failed to compile.

./src/App.tsx
MacroError: Invalid import: DefaultTheme. You can only import default, css, keyframes, createGlobalStyle, isStyledComponent, ThemeConsumer, ThemeContext, ThemeProvider, withTheme, ServerStyleSheet, StyleSheetManager, __DO_NOT_USE_OR_YOU_WILL_BE_HAUNTED_BY_SPOOKY_GHOSTS from 'styled-components/macro'.

DefaultTheme is a type exported by styled-components, which we're augmenting with our theme's types.

[awto]

Why wouldn't you just put the macros plugin after the TS? Or as an alternative implement TS syntax imports in the macros plugin.

But this behavior now is actually more predictable, if macros are applied before some syntax plugin the macros will work with that syntax. What if I want to make a macro for TS code, for example.

[mAAdhaTTah]

@awto With create-react-app, we don't have control over the order of the plugins. If you think this should be solved upstream, I can open an issue there.

[awto]

I think so. The original problem is not the order, it would be fine if it runs strictly after all the passes always. The problem was non-determinism in the order. Any plugin running in between could break something working after something maybe small was changed in that plugin. But I'm not sure if it is for CRA or styled-components.

[mAAdhaTTah]

I definitely narrowed down the issue to changing to this version of babel-plugin-macros. However, it appears the typescript plugin is defined as a preset vs macros as a plugin. I'm not sure how to arrange the order such that macros comes after typescript.

[awto]

I don't object this change broke your config, but only because of the original working version was based on that time random ordering. Any change in typescript plugins or some plugins going after in the chain could break the same anytime. With the change, this shouldn't happen.

It is possible to turn a plugin into a preset by just listing it plugins property, something like this:

module.exports = {
   plugins: [require("babel-plugin-macros")]
}

this way, or if the configuration is already javascript such object can be added into a presets list. And so it gives better passes ordering control.

[mAAdhaTTah]

The issue isn't so much that other plugins can add things so much as previously TS stripped things that the macro can't operate on. Moving the plugin -> presets didn't seem to resolve the issue, but still looking that route.

In any case, I opened an issue with SC here: https://github.com/styled-components/styled-components/issues/2378

FWIW, I know plugin ordering has been a sticky problem for Babel for a while; definitely tough to solve.

[kentcdodds]

FWIW, I know plugin ordering has been a sticky problem for Babel for a while; definitely tough to solve.

https://jamie.build/babel-plugin-ordering.html

[mAAdhaTTah]

@kentcdodds Yeah, that kinda suggests the SC macro shouldn't validate the imports.

[kentcdodds]

Seems more like they should include the type defs in their validation.

[mAAdhaTTah]

Also a possibility I suggested. That said, most of what I've seen from them RE TypeScript is that they don't use TypeScript so they don't maintain the typings and have pushed that off to the community. Which is fine! But may engender some resistance here.

Anyway, I have an issue open over there, so we'll see what happens. Thanks for your time, both of you!