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

Possibility to add babel plugins via registry #42

Closed
wants to merge 2 commits into from
Closed

Possibility to add babel plugins via registry #42

wants to merge 2 commits into from

Conversation

pangratz
Copy link
Member

This allows Ember-CLI addons to add custom babel transformations to the
ember-cli-babel pipeline by registering a plugin using the
babel-plugin type:

// my-addon/index.js
module.exports = {
  name: 'my-addon-using-babel-transformer',

  setupPreprocessorRegistry: function(type, registry) {
    var BabelTransformerPlugin = require('./babel-transformer-plugin');

    registry.add('babel-plugin', {
      name: 'my-addon-using-babel-transformer',
      plugin: BabelTransformerPlugin
    });
  }
}

// my-addon/babel-transformer-plugin.js
module.exports = function(babel) {
  var t = babel.types;
  return new babel.Transformer('troll-transformer', {
    Literal: function(node) {
      if (node.value === true) {
        return t.literal(false);
      } else if (node.value === false) {
        return t.literal(true);
      }
    }
  });
}

I will add a section to the README if this is accepted.

This addresses ember-cli/ember-cli-htmlbars-inline-precompile#7

@stefanpenner
Copy link
Member

mind adding a test of some kind?

@rwjblue
Copy link
Member

rwjblue commented May 20, 2015

@pangratz - This looks good.

index.js Outdated
// add babel-plugins from the registry to the options.plugins
var pluginWrappers = addonContext.registry.load('babel-plugin');
var babelPlugins = pluginWrappers.map(function(wrapper) {
return wrapper.plugin;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the property under which the transformer is looked up be changed to transformer? I copied this from the ember-cli-htmlbars addon, where AST plugins are added like this ...

Copy link
Member

Choose a reason for hiding this comment

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

I think they are called plugins in babel speak also, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://babeljs.io/docs/usage/plugins/ yep, I think it's ok then

@pangratz
Copy link
Member Author

mind adding a test of some kind?

done

@pangratz
Copy link
Member Author

Do NOT merge yet, I have to check if the plugin lookup works as expected ...

index.js Outdated
@@ -52,6 +52,15 @@ function getOptions(addonContext) {
}
}

// add babel-plugins from the registry to the options.plugins
var pluginWrappers = addonContext.registry.load('babel-plugin');
Copy link
Member Author

Choose a reason for hiding this comment

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

Am I right that using addonContext.registry only looks up plugins registered within this addon, aka in-repo-addons? Because if I'm using this in ember-cli-htmlbars-inline-precompile and register a babel-plugin it won't be picked up. Using addonContext.app.registry instead correctly picks up the plugins registered in other addons. @rwjblue can you confirm this?

Copy link
Member

Choose a reason for hiding this comment

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

You want addonContext.parent.registry (addonContext.app is not always available).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, using addonContext.parent.registry.load('babel-plugin') gives me a Cannot read property 'load' of undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would something like currently used to get the options be ok?

var registry = (addonContext.parent && addonContext.parent.registry) || (addonContext.app && addonContext.app.registry);
var pluginWrappers = registry.load('babel-plugin');
...

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer that we stash the registry instance we are passed for parent in the setupPreprocessorRegistry hook.

Something like:

setupPreprocessorRegistry: function(type, registry) {
  if (type === 'parent') {
    this.parentRegistry = registry;
  }
}

Then you should be able to call addonContext.parentRegistry.load without issue (or hacks).

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice!

@pangratz
Copy link
Member Author

Ok, so from my side this is good to go. I tested this within another addon and adding babel-plugins to the transformation pipeline works as expected 🙌

@rwjblue
Copy link
Member

rwjblue commented May 21, 2015

WOOT

@rwjblue
Copy link
Member

rwjblue commented May 22, 2015

@stefanpenner - Any additional feedback / tweaks?

@stefanpenner
Copy link
Member

I'll take a thorough look today. On my way to work now

@rwjblue
Copy link
Member

rwjblue commented May 24, 2015

@stefanpenner - Any concerns after your review?

@stefanpenner
Copy link
Member

have not had time to review.

@pangratz
Copy link
Member Author

Since babel is moving so fast and features are added on an hourly basis, should there be some kind of check if the currently installed babel version satisfies the needs of the specific plugin? Something like:

registry.add('babel-plugin', {
  name: 'my-addon-using-babel-transformer',
  plugin: BabelTransformerPlugin,
  babelVersion: '5.2.10'
});

Where the required babel version might also be extractable from the addons' package.json ...


Currently all registered plugins of babel-plugin type are added to the pipeline. This means that also the answer-to-the-universe plugin of this PR is added (I will check if this is true in a sample app later). If there is a string like THE-answer in the app, it will be replaced by 42...

@rwjblue
Copy link
Member

rwjblue commented Jun 1, 2015

Having the allowed babel version range be required sounds nice. We could then use the semver package to ensure we are in compliance of that range (and if not warn + skip).

Though, I think that could be done in a follow-up PR if @stefanpenner has had a chance to review this and is OK with it landing...

@pangratz
Copy link
Member Author

pangratz commented Jun 1, 2015

Having the allowed babel version range be required sounds nice. We could then use the semver package to ensure we are in compliance of that range (and if not warn + skip).

👍

@rwjblue
Copy link
Member

rwjblue commented Jun 1, 2015

@stefanpenner - Ping

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

I spoke with @stefanpenner about this at our last face to face meeting. We need an example app that has addons with differing versions of ember-cli-babel and using different plugins to make sure everything is handled correctly. Double checking that things are not shared is the main blocker for this PR.

@pangratz - Do you think that you might be able to put together a demo app like that?

@pangratz
Copy link
Member Author

I'm on it

@pangratz
Copy link
Member Author

@rwjblue @stefanpenner I created a sample app at https://github.com/pangratz/ember-cli-babel-plugins-sample, which uses the ember-cli-babel version of this PR. Two addons are installed:

The sample app has a test case, which can only succeed when both plugins work (the first plugin replaces babel-plugin-demo1 with demo1; and the second replaces babel-plugin-demo2 with demo2).

As the Travis run shows, everything works as expected...

@stefanpenner
Copy link
Member

@pangratz this is a great start, thank you.

The test case should at the very least, include another level of addons.

app -> my-cool-addon -> some-babel-plugin

some-babel-plugin should affect the addon/ directory of my-cool-addon but not the host application. my-cool-addon may also choose to op into different babel options.

@pangratz
Copy link
Member Author

my-cool-addon may also choose to op into different babel options.

Do you have an idea how to check that options enabled in my-cool-addon are not available/enabled in the host application?

@stefanpenner
Copy link
Member

Do you have an idea how to check that options enabled in my-cool-addon are not available/enabled in the host application?

one idea, is to disable something like arrow functions in the parent, but enable them in one of the children. This idea should translate to babel-plugins as well.

Two children addons, should be able to have deps on the same babel-plugin, but configure them differently.

@pangratz
Copy link
Member Author

😳 I think I need help on how to set/overwrite the ember-cli-babel options from inside an addon. Where is the right place for that?

@stefanpenner
Copy link
Member

I think I need help on how to set/overwrite the ember-cli-babel options from inside an addon. Where is the right place for that?

I am pressed for time today, if you can put together an app that has the 3 layers described. (even if the options stuff doesn't work yet) I will attempt to dedicate some hours this weekend to it :)

Once that lands, we can guage our current state.

There may be a scenario, where we do a two step release. Releasing the more complex scenario later, I would prefer we don't though. If we must, i would like to have a clear path set.

@pangratz
Copy link
Member Author

Ok, I gave it a try and removed the babel-plugin-demo2 dependency from the sample app and created a babel-demo-addon which depends on the babel-plugin-demo2 addon.

The babel-demo-addon provides a {{my-component}}, which has a leProperty which is replaced via the babel-plugin-demo2: pangratz/babel-demo-addon@b6cb25a

The sample app uses babel-demo-addon to check if the babel-plugin-demo2 is correctly setup for within this addon, but not for the sample app itself, which it seems that currently doesn't work as expected (failing travis run for pangratz/ember-cli-babel-plugins-sample@ae80ab7).


I hope I understood you correctly regarding setup up the hierarchy and gave you both access to the relevant repos so you can check around if you have time ...

@stefanpenner
Copy link
Member

@pangratz I will confirm, but I believe this is the scenario I was concerned this PR missed.

@stefanpenner
Copy link
Member

there is a path, where we merge this in early. ANd explicity disallow the addon case, or hard-wire some of the common plugins we want.

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2015

Let's just have a separate PR explicitly add the inline precompiler for now. I want to unblock the component integration test situation....

@stefanpenner
Copy link
Member

I want to unblock the component integration test situation....

👍

@pangratz
Copy link
Member Author

Let's just have a separate PR explicitly add the inline precompiler for now

Will create such a PR tomorrow ...

@pangratz
Copy link
Member Author

Instead of creating a new PR I've updated this one by only adding whitelisted babel-plugins to the transformation pipeline (currently only ember-cli-htmlbars-inline-precompile)...

If you guys are ok with updating this PR, I will update the README accordingly and squash the commits.

@stefanpenner
Copy link
Member

@rwjblue r?

@pangratz
Copy link
Member Author

pangratz commented Jul 5, 2015

@rwjblue did you have time to think about the proposed interim solution in this PR?

@pangratz
Copy link
Member Author

pangratz commented Oct 9, 2015

I haven't followed the latest developments in ember-cli, but have there been any changes regarding the addon-dependency-situation?

@ffaubry
Copy link

ffaubry commented Dec 1, 2015

Very interested by this PR... what's the status?

This allows Ember-CLI addons to add custom babel transformations to the
ember-cli-babel pipeline by registering a plugin using the
`babel-plugin` type:

``` javascript
// my-addon/index.js
module.exports = {
  name: 'my-addon-using-babel-transformer',

  setupPreprocessorRegistry: function(type, registry) {
    var BabelTransformerPlugin = require('./babel-transformer-plugin');

    registry.add('babel-plugin', {
      name: 'my-addon-using-babel-transformer',
      plugin: BabelTransformerPlugin
    });
  }
}

// my-addon/babel-transformer-plugin.js
module.exports = function(babel) {
  var t = babel.types;
  return new babel.Transformer('troll-transformer', {
    Literal: function(node) {
      if (node.value === true) {
        return t.literal(false);
      } else if (node.value === false) {
        return t.literal(true);
      }
    }
  });
}
```
In the previous commit a babel-plugin registered on the registry is
added to the transformation pipeline. Since this doesn't work as
expected for the case where an addon depends on such a babel-plugin but
the app which uses the addon doesn't include the babel-plugin, we
currently only support a whitelist of specific plugins until this is
figured out and eventually work.

This commit adds `ember-cli-htmlbars-inline-precompile` to the
whitelisted plugins and adds a test that other registered plugins are
not added to the transformation pipeline.
@Cryrivers
Copy link

same question as above.

@lucasm-iRonin
Copy link

What's the status? I would like to integrate babel-plugin-typecheck to my Ember apps and allowing for custom babel plugins is the only option I'm aware of.

@Iftahh
Copy link

Iftahh commented Mar 31, 2016

+1
very interested in this

@tschoartschi
Copy link

I'm also interested in the status of this pull request :)

@mguida22
Copy link

mguida22 commented Jun 9, 2016

An update on this would be great! 😄

@tschoartschi
Copy link

I would really need an update on this issue. Because if it is not going to happen in the near future I'll have to rearrange my work packages. It is no problem but I just need to know. Thanks :)

@tschoartschi
Copy link

tschoartschi commented Jun 23, 2016

@mguida22 @lucasm-iRonin @Cryrivers @ffaubry @Iftahh what I ended up doing is the following: I created an in-repo-addon and applied my babel-transformations after the ember-cli-babel plugin and before ember-cli-uglify. For me this works fine, maybe this is a solution for you as well

@rwjblue
Copy link
Member

rwjblue commented Jan 7, 2020

Going to close this for now...

@rwjblue rwjblue closed this Jan 7, 2020
siva-sundar pushed a commit to siva-sundar/ember-cli-babel that referenced this pull request Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants