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

fix unused helper stripping #316

Merged
merged 1 commit into from
Apr 24, 2019
Merged

fix unused helper stripping #316

merged 1 commit into from
Apr 24, 2019

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Apr 18, 2019

This fixes #308.

As there is no existing test infrastructure for running builds with different build-time options, there was not a good place to put a test so I didn't make one.

The changes here fix both parts of the problem reported in #308.

  • in the addon tree, we need to rewrite the index.js file so it stops trying to import helpers that have already been removed.
  • in the app tree, we need to remove the files that reexport removed helpers.

treeForAddon: function(tree) {
tree = this.filterHelpers(tree, new RegExp(/^helpers\//, 'i'));
tree = new StripBadReexports(tree, [`index.js`]);
return this._super.treeForAddon.call(this, tree);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Life is easier if you process the tree before it has run through _super.treeForAddon. For one thing, you no longer need to worry about whether your current version of ember-cli includes the modules subdir (which is why the original RegExp was more complex). For another, we want our StripBadReexports plugin to process the import statements before they have been compiled into AMD by ember-cli-babel.

@@ -38,7 +43,7 @@ module.exports = {

// exit early if no opts defined
if ((!whitelist || whitelist.length === 0) && (!blacklist || blacklist.length === 0)) {
return new Funnel(tree);
return tree;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new Funnel(tree) is a no-op.

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Tested on a few apps and works! You are amazing @ef4. Thank you. I learned a little bit as well.

@snewcomer snewcomer merged commit bf0d45c into DockYard:master Apr 24, 2019
ctjhoa added a commit to concordnow/ember-cli-string-helpers that referenced this pull request Mar 24, 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.

only and except leave broken module references and unnecessary code in app tree
2 participants