Skip to content
This repository has been archived by the owner. It is now read-only.

Ember CLI: Production code stripping #50

Merged

Conversation

@GavinJoyce
Copy link
Contributor

GavinJoyce commented Apr 6, 2016

Rendered

some previous discussion

@webark
Copy link

webark commented Apr 6, 2016

All calls to the methods of those that you mentioned get stripped out in a production build except for Ember.info. And I'd say that (even though it might be contentious) there are reasons to use Ember.info in a production build. (that's the main difference between that and Ember.debug) So I'd say that one should get removed from the list.

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Apr 6, 2016

I don't believe that they are stripped from production, AFAIK the functions calls remain. They are no-ops though
On 6 Apr 2016 5:48 p.m., "Mark Avery" notifications@github.com wrote:

All calls to the methods of those that you mentioned get stripped out in a
production build except for Ember.info. And I'd say that (even though it
might be contentious) there are reasons to use Ember.info in a production
build. (that's the main difference between that and Ember.debug) So I'd
say that one should get removed from the list.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#50 (comment)

@webark
Copy link

webark commented Apr 6, 2016

Oh. In the documentation it sates

Ember build tools will remove any calls to Ember.debug() when doing a production build.

And for info it does not say that, just says Display an info notice., but in the code it looks like it might treat them the same. https://github.com/emberjs/ember.js/blob/v2.4.4/packages/ember-debug/lib/index.js#L89-L97

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Apr 6, 2016

Oh. In the documentation it sates

I recently updated the Ember.assert docs: emberjs/ember.js#13180 as it was incorrect. It's possible that the docs are incorrect elsewhere too

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Apr 6, 2016

.. there are reasons to use Ember.info in a production build

I don't use it but I can see why others would. We could leave Ember.info alone

@webark
Copy link

webark commented Apr 6, 2016

I never have either, but was just basing it off of the docs. However, looking at the code...

/**
  Display an info notice.
  @method info
  @private
*/
setDebugFunction('info', function info() {
  Logger.info.apply(undefined, arguments);
});

it seems like it's treated the same. Just looks like the docs for that need to be updated.

@rwjblue
Copy link
Member

rwjblue commented Apr 6, 2016

@webark - Ember.info should be stripped, but if you do want info statements in production you should use Ember.Logger.info.


TODO:

* [ ] Are all these functions already no-ops in production? If so, there are no obvious drawbacks

This comment has been minimized.

Copy link
@GavinJoyce

GavinJoyce Apr 6, 2016

Author Contributor

Thanks, updated

@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/production-code-stripping branch from 855c04e to 8a40169 Apr 6, 2016
@webark
Copy link

webark commented Apr 6, 2016

@rwjblue good to know. It looks like the docs need to be updated to reflect that. I might find out where that is and go and issue a PR.

@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/production-code-stripping branch 2 times, most recently from d3e11d7 to 17effe9 Apr 6, 2016
@runspired
Copy link

runspired commented Apr 6, 2016

I believe this belongs being a feature of Ember proper once Ember itself is an addon, or as an official but stand alone addon, and not part of ember-cli. My reasoning is that ember-cli is increasingly not ember-specific, but this behavior is very specific to Ember apps.

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Apr 6, 2016

ember-cli is increasingly not ember-specific, but this behavior is very specific to Ember apps

@runspired Perhaps other consumers of ember-cli would also benefit from the ability to configure what to strip from production builds? Only the configuration for ember builds needs to be ember specific

@nathanhammond
Copy link
Contributor

nathanhammond commented Apr 6, 2016

@runspired Regardless of where it lives (officially sanctioned and bundled addon, core ember-cli) this is the right place to have the conversation.

The question is almost "should addons be responsible for this themselves, or should ember-cli be a cool cat and have your back?" It can be built-in, a dep for ember-cli, or an individual dep for each addon that needs it. Since Ember is a future addon we have a way to think about handling it for Ember (note that this is one of a few build tasks for Ember itself which needs logic and will need some way to handle that when not being bundled as a holistic package).

I don't feel particularly strongly about where it lives but tend toward including it as a dependency of ember-cli. This sort of behavior (if configurable) will be used across all future things similar to ember-cli.

@runspired
Copy link

runspired commented Apr 7, 2016

@GavinJoyce @nathanhammond if its abstract in a way that lots of addons can use it, and it just happens to be configured for Ember, that's much different than building in something that strips out code that's closely tied to Ember. I'm all for the abstract thing being in ember-cli :)

* [babel-plugin-filter-imports](https://github.com/ember-cli/babel-plugin-filter-imports) with added support for defining module functions for removal ([spike here](https://github.com/GavinJoyce/babel-plugin-filter-imports/pull/1))
* a new babel plugin which works similarly to `babel-plugin-filter-imports`

For consistency, this plugin should also be used in Ember Data and other Ember projects which would benefit from the reduced size.

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Apr 17, 2016

Contributor

the detailed design should include how this works with addons, and nested addons.

This comment has been minimized.

Copy link
@mixonic
@stefanpenner
Copy link
Contributor

stefanpenner commented Apr 17, 2016

@krisselden this is related to your thoughts/ideas you should likely provide feedback.

@les2
Copy link

les2 commented Oct 30, 2016

One scary side-effect of not stripping Ember.assert, et al, from production builds is that the arguments to those functions will not be lazily evaluated.

So even though the functions are implemented as no-ops in production builds, depending on what arguments that are slow to evaluate or have side-effects could cause either performance problems or bugs when run in production.

Before reading this RFC, I'd been operating under the assumption that Ember.assert calls would be stripped from production builds.

As a work around, we should perhaps encourage anyone using Ember.assert et al to wrap those in a boolean such as Ember.isDebug (assuming something like this exists):

performanceCriticalFunction() {
    ...
    if (Ember.isDebug) { // <--- at least the arguments won't be evaluated when in production, strip-or-no-strip
       Ember.assert('someArray should include someValue', someArray.includes(someValue));
    }
}

Short of AST-hackery or C-style macros, an if guard is necessary unless a language supports lazy evaluation of arguments.

Until this is implemented, perhaps we should encourage projects like ember-data to wrap their debug code in such a guard.

Aside: A more generic mechanism could be used for ember-cli compile time build stripping. For example, code that exists only to provide compatibility with older versions of ember could be stripped when included in newer versions (kinda like macros in C):

if (Ember.Version.range('1.13', '2.4')) {
  Foo.reopen({ ... }); // <--- this would get stripped if the ember version at build time is 2.5+
}

Thoughts?

@stefanpenner
Copy link
Contributor

stefanpenner commented Oct 31, 2016

@les2 yup, we agree / are aware. Would love to see this land

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Nov 2, 2016

@les2 I've got an implementation of this RFC which I'll extract to an addon soon. I'll post again when it's available

@rwjblue
Copy link
Member

rwjblue commented Nov 4, 2016

I just did a fresh read through, and think we should also publicly expose helpers similar to the ones used inside Ember itself for testing around assertions (in Ember we use emberjs/ember-dev to provide expectAssertion / expectWarn / expectDeprecation etc).

Other than that, I am pretty happy with this moving forward...

@mixonic
Copy link
Member

mixonic commented Nov 7, 2016

Great, shipping this to FCP today!

@runspired
Copy link

runspired commented Nov 7, 2016

I've written a few babel5/babel6 plugins to do this sort of production code stripping in multiple addons now. I would be very willing to help build an abstract version to land this RFC. :)

@Gaurav0
Copy link
Member

Gaurav0 commented Nov 7, 2016

Definitely in favor of this. @runspired, is there an addon I can use to have this happen in my app today?

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Nov 7, 2016

@runspired Nice one. I'm planning to work on an experimental implementation of this later today, I'll post the repo when there is something in it.

@Gaurav0 This should be relatively easy to build, I expect that there will be something to try out tomorrow. I'm hoping to run the early experimental addon in Intercom in production later this week.

* [`Ember.deprecate`](http://emberjs.com/api/#method_deprecate)
* [`Ember.info`](http://emberjs.com/api/#method_info)
* [`Ember.runInDebug`](http://emberjs.com/api/#method_runInDebug)
* [`Ember.warn`](http://emberjs.com/api/#method_warn)

This comment has been minimized.

Copy link
@les2

les2 Nov 8, 2016

is it wise to strip warn? if i were new to ember i would assume that info and "higher" were not stripped.

edit: the wip implementation is configurable so this concern is no longer relevant.

This comment has been minimized.

Copy link
@mixonic

mixonic Nov 11, 2016

Member

@les2 Also both warn and info are already no-op in production builds. Stripping them don't change any behavior.

The idea of log level here is mostly related to how these things appear in the JS console in browsers, not to what build of ember that function emits in.


# Unresolved questions

* [ ] Should we allow the stripping configuration to be overridden in `ember-cli-build.js`? If so, what should the API be?

This comment has been minimized.

Copy link
@les2

les2 Nov 8, 2016

for info and debug statements, it would be useful to configure them to not be stripped per add-on.

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Nov 11, 2016

Contributor

Yes, this should be configurable. The API should require you to identify source (global, module), and then the corresponding keypath. In other words, enough to feed it directly into Babel. The core rules should be implemented in this manner.

This comment has been minimized.

Copy link
@GavinJoyce

GavinJoyce Nov 12, 2016

Author Contributor

How about:

{
  removals: [
    {
      module: 'ember', //eg. import Em from 'ember';
      paths: [
        'assert', //Em.assert will be removed
        'debug',  //Em.debug will be removed
        'a.b.c'   //Em.a.b.c will be removed
      ]
    }, {
      global: 'Ember',
      paths: [
        'deprecate' //Ember.deprecate will be removed
      ]
    }, {
      paths: [
        'console.log' //console.log will be removed
      ]
    }
  ]
}

I'd like to find a better key name than removals.

A Babel plugin will execute the removal of these function calls. It will either be:

* [babel-plugin-filter-imports](https://github.com/ember-cli/babel-plugin-filter-imports) with added support for defining module functions for removal ([spike here](https://github.com/GavinJoyce/babel-plugin-filter-imports/pull/1))
* a new babel plugin which works similarly to `babel-plugin-filter-imports`

This comment has been minimized.

Copy link
@Turbo87

Turbo87 Nov 8, 2016

Member

note that there is also https://github.com/azu/babel-plugin-strip-function-call, but it is Babel 6+

This comment has been minimized.

Copy link
@GavinJoyce

GavinJoyce Nov 8, 2016

Author Contributor

Thanks. This one doesn't seem to handle import statements, but it is useful for reference

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Nov 8, 2016

WIP experimental implementation of the babel transform: https://github.com/GavinJoyce/babel-plugin-remove-functions

@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Nov 10, 2016

Perhaps we should also enable by default on test builds (or perhaps only disabled on development builds)? Any side effects that removing the code might introduce on production would also be tested

@rwjblue
Copy link
Member

rwjblue commented Nov 10, 2016

I don't think so. The main reason is that you want all of the helpful assertions/deprecations to fire in tests (so you can fix them and have a fighting chance at understanding what is going on).

I have long advocated that folks should always be running their apps tests against production builds in CI. Typically I suggest a matrix setup where you run both ember test and ember test --environment=production.

* [babel-plugin-filter-imports](https://github.com/ember-cli/babel-plugin-filter-imports) with added support for defining module functions for removal ([spike here](https://github.com/GavinJoyce/babel-plugin-filter-imports/pull/1))
* a new babel plugin which works similarly to `babel-plugin-filter-imports`

The babel plugin will affect the code of the current app or addon only and won't affect code in child or grandchild addons. As this change becomes part of the default ember-cli configuration, addons will adopt the code stripping as they upgrade to newer ember-cli versions.

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Nov 11, 2016

Contributor

How are you intending to accomplish this? Are you intending to register it as a preprocessor? This feels relatively tricky to do right.

This comment has been minimized.

Copy link
@rwjblue

rwjblue Nov 11, 2016

Member

Pretty easy (stolen from ember-cli-htmlbars-inline-precompile):

 included: function(app) {
    this._super.included.apply(this, arguments);

    app.options = app.options || {};
    app.options.babel = app.options.babel || {};
    app.options.babel.plugins = app.options.babel.plugins || [];

    if (!this._registeredWithBabel) {
      app.options.babel.plugins.push(require('./some-path-to-thing');
      this._registeredWithBabel = true;
    }
}

When added as a dependency, this will add to its including project's or addon's babel processing (which is independent at each layer).

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Nov 11, 2016

Contributor

That's pretty clever, actually. I like it.


# Unresolved questions

* [ ] Should we allow the stripping configuration to be overridden in `ember-cli-build.js`? If so, what should the API be?

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Nov 11, 2016

Contributor

Yes, this should be configurable. The API should require you to identify source (global, module), and then the corresponding keypath. In other words, enough to feed it directly into Babel. The core rules should be implemented in this manner.

# Unresolved questions

* [ ] Should we allow the stripping configuration to be overridden in `ember-cli-build.js`? If so, what should the API be?
* [ ] Related to above, should we allow the feature to be disabled?

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Nov 11, 2016

Contributor

Making this possible makes for annoying partial config in ember-cli-build.js (which things to strip) and in environment.js (which environments to strip in) and will be a source of numerous user-error bugs.

Since we will ship with defaults of some sort we do need to provide a way to modify/clobber those, and whatever that method is will be the way to turn it off (or just remove the addon). (Keeps the API cleaner.)

This comment has been minimized.

Copy link
@rwjblue

rwjblue Nov 11, 2016

Member

Disabling should be on a per-addon/project basis, by simply removing the addon.

@nathanhammond
Copy link
Contributor

nathanhammond commented Nov 11, 2016

Cleanup things I'd like to make a bit more clear but don't change anything:

  • Squash, rebase.
  • Note that it must account for destructured and reassigned invocations of these functions.
  • Stated intent to support both Babel 5/Babel 6.
@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/production-code-stripping branch from af60ace to 7362ad0 Nov 12, 2016
@GavinJoyce
Copy link
Contributor Author

GavinJoyce commented Nov 12, 2016

@nathanhammond thanks, I've completed those tasks. I've also added an example of possible configuration

@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/production-code-stripping branch from 7362ad0 to 6e4cdbe Nov 12, 2016
@nathanhammond
Copy link
Contributor

nathanhammond commented Nov 16, 2016

Because of the updates regarding configuration we're restarting FCP. Barring additional commentary this is on the agenda for approval for the November 24 Ember CLI meeting.

No meeting on November 24 because of US Thanksgiving. December 1.

@nathanhammond
Copy link
Contributor

nathanhammond commented Dec 2, 2016

No further feedback, we're accepting this RFC and intending to land an implementation. Thank you @GavinJoyce for instigating this and bring it to completion!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.