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

Strip stuff from addon before it is added to app #4048

Merged
merged 1 commit into from
Jan 9, 2016
Merged

Strip stuff from addon before it is added to app #4048

merged 1 commit into from
Jan 9, 2016

Conversation

pangratz
Copy link
Member

@pangratz pangratz commented Jan 5, 2016

Currently the code of the ember-data addon is added to the app. This is problematic since the code contains assert, debug, etcetera calls and also no feature flag code is stripped.

This PR is my stab at fixing this, so only the stripped code is added via the treeForAddon hook in index.js.

This addresses #4047 and takes inspiration from @fivetanley's gist.


TODO

  • Check if this works as expected
  • Only strip debug statements imported from ember-data/-private/debug if environment === "production"? Currently the statements are always stripped.
  • Correctly strip features which have a value other than true in config/features.json. will be addressed in an upcoming PR

@pangratz
Copy link
Member Author

pangratz commented Jan 5, 2016

Ok, so I tested this locally:

# checkout ember-data from PR and link to npm
git clone https://github.com/emberjs/data.git
cd data
hub checkout https://github.com/emberjs/data/pull/4048
npm link

# create new ember app using linked ember-data
cd ..
ember new my-app --skip-bower --skip-npm
cd my-app
bower install
bower uninstall ember-data --save
bower install ember-cli/ember-cli-shims#0.1.0 --save
npm link ember-data
npm install

# create build and check if dist/assets/vendor.js contains correctly stripped ember-data
ember build

All the debug statements from ember-data/-private/debug are correctly stripped (and the feature-flagged stuff is still present behind if(isEnabled("ds-xxx")) {}, which is expeceted).

There is a problem though: all the ember-data modules in dist/assets/vendor.js are inside additional define('ember-data/xxx', ...) calls (see full vendor.js):

define('ember-data/-private/adapters/build-url-mixin', function () {

  'use strict';

  define('ember-data/-private/adapters/build-url-mixin', ['exports', 'ember'], function (exports, _ember) {

    var get = _ember['default'].get;

    // ...
  });

});

EDIT: the problem has been fixed with help from the sharp eyes of @rwjblue.

@@ -49,10 +49,20 @@ module.exports = {
return { inputTree: dir, rebuild: function() { return []; } };
}

var version = require('./lib/version');
var merge = require('broccoli-merge-trees');
// TODO only return stripped code if we are in production environment?
Copy link
Member

Choose a reason for hiding this comment

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

if (process.env.EMBER_ENV !== 'production') {
  var strippedAddonCode = strippedBuild('ember-data', dir, {
    blacklist: ['es6.modules', 'useStrict']
  });

  return this._super.treeForAddon.call(this, strippedAddonCode);
} else {
  return this._super.treeForAddon.call(this, merge([version(), dir]));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

🙏

@rwjblue
Copy link
Member

rwjblue commented Jan 6, 2016

This looks good to me, and I'd like to see this land even without the features.json work done. The debug statement stripping in production will affect [email protected] when we release it, but since 2.3.x doesn't include features yet the features.json portion does not have an impact on 2.3.0.

Thoughts on landing this into beta, and iterating on it for 2.4.0-beta.x series for the feature flag stripping?

@bmac
Copy link
Member

bmac commented Jan 6, 2016

@pangratz do you mind squashing these commits and prefix the commit message with [BUGFIX beta]

@eriktrom
Copy link
Contributor

eriktrom commented Jan 9, 2016

@pangratz - this doesn't work unless the following are moved to dependencies (instead of devDependencies) - at least that's what I found (im using ember-cli-deploy though)

Here's a compare: https://github.com/pangratz/data/compare/fix-ember-addon-production...eriktrom:fix-ember-addon-production

Otherwise, looks good and works for me

Thanks for tackling this

@pangratz
Copy link
Member Author

pangratz commented Jan 9, 2016

Awesome catch @eriktrom! Thanks for testing!

Looks like a ember build --prod also doesn't work when the dependencies are only available in devDependencies. I've updated this PR to move the dependencies which are needed for a ember build --prod into dependencies and now it works:

ember new my-app --skip-bower --skip-npm
cd my-app
bower install
bower uninstall ember-data --save
bower install ember-cli/ember-cli-shims#0.1.0 --save
npm install pangratz/data#fix-ember-addon-production
npm install

ember build --prod

bmac added a commit that referenced this pull request Jan 9, 2016
Strip stuff from addon before it is added to app
@bmac bmac merged commit 4db5365 into emberjs:master Jan 9, 2016
@bmac
Copy link
Member

bmac commented Jan 9, 2016

Thanks @pangratz and @eriktrom

@pangratz pangratz deleted the fix-ember-addon-production branch January 9, 2016 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants