Ember CLI: Production code stripping #50

GavinJoyce · 2016-04-06T15:58:28Z

webark · 2016-04-06T16:48:38Z

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 · 2016-04-06T17:04:33Z

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 · 2016-04-06T17:11:49Z

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 · 2016-04-06T17:14:29Z

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 · 2016-04-06T17:15:39Z

.. 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 · 2016-04-06T17:17:24Z

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 · 2016-04-06T18:20:11Z

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

rwjblue · 2016-04-06T18:22:30Z

active/0000-production-code-stripping.md

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


Yes, all of the functions above are set to function() { } by default:

https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/core.js#L52-L58

https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/debug.js#L1-L10

Thanks, updated

webark · 2016-04-06T18:43:29Z

@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.

runspired · 2016-04-06T20:15:43Z

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 · 2016-04-06T20:27:13Z

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 · 2016-04-06T23:12:37Z

@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 · 2016-04-07T00:15:03Z

@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 :)

stefanpenner · 2016-04-17T20:28:14Z

active/0000-production-code-stripping.md

+ * [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.


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

stefanpenner · 2016-04-17T20:42:18Z

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

les2 · 2016-10-30T22:54:42Z

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 · 2016-10-31T14:56:23Z

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

GavinJoyce · 2016-11-02T08:58:27Z

@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 · 2016-11-04T17:46:58Z

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 · 2016-11-07T17:47:37Z

active/0000-production-code-stripping.md

@@ -28,17 +28,23 @@ A Babel plugin will execute the removal of these function calls. It will either
 * [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.
+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.


@stefanpenner @rwjblue please review this change to the code stripping RFC

Yep, this seems like exactly what we discussed.

mixonic · 2016-11-07T18:20:50Z

Great, shipping this to FCP today!

runspired · 2016-11-07T18:31:03Z

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 · 2016-11-07T18:38:47Z

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

GavinJoyce · 2016-11-07T18:51:16Z

@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.

les2 · 2016-11-08T02:40:31Z

active/0000-production-code-stripping.md

+ * [`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)


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.

@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.

les2 · 2016-11-08T02:53:22Z

active/0000-production-code-stripping.md

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


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

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.

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.

Turbo87 · 2016-11-08T08:07:21Z

active/0000-production-code-stripping.md

+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`


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

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

GavinJoyce · 2016-11-08T16:29:10Z

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

GavinJoyce · 2016-11-10T15:09:39Z

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 · 2016-11-10T15:15:36Z

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.

nathanhammond · 2016-11-11T00:24:06Z

active/0000-production-code-stripping.md

+ * [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.


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

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).

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

nathanhammond · 2016-11-11T00:27:07Z

active/0000-production-code-stripping.md

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


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.

nathanhammond · 2016-11-11T00:35:48Z

active/0000-production-code-stripping.md

+# 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?


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.)

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

nathanhammond · 2016-11-11T00:40:40Z

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 · 2016-11-12T17:50:18Z

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

nathanhammond · 2016-11-16T18:55:20Z

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 · 2016-12-02T07:25:29Z

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!

GavinJoyce force-pushed the gj/production-code-stripping branch from 43f4094 to 855c04e Compare April 6, 2016 15:58

rwjblue reviewed Apr 6, 2016
View reviewed changes

GavinJoyce force-pushed the gj/production-code-stripping branch from 855c04e to 8a40169 Compare April 6, 2016 18:41

GavinJoyce force-pushed the gj/production-code-stripping branch 2 times, most recently from d3e11d7 to 17effe9 Compare April 6, 2016 19:06

webark mentioned this pull request Apr 6, 2016

[DOC release] updated docs regarding ember debug methods emberjs/ember.js#13272

Merged

stefanpenner reviewed Apr 17, 2016
View reviewed changes

rwjblue mentioned this pull request Jun 7, 2016

createDeprecatedModule should be striped from production builds. ember-cli/ember-resolver#132

Open

nathanhammond mentioned this pull request Jun 9, 2016

[BUGFIX] Remove references to Ember.Logger.assert emberjs/guides#1467

Merged

aars mentioned this pull request Aug 29, 2016

Improve test setup pixelhandler/ember-jsonapi-resources#92

Merged

mixonic reviewed Nov 7, 2016

View reviewed changes

GavinJoyce added the Final Comment Period label Nov 7, 2016

les2 reviewed Nov 8, 2016

View reviewed changes

Turbo87 reviewed Nov 8, 2016

View reviewed changes

nathanhammond reviewed Nov 11, 2016

View reviewed changes

GavinJoyce force-pushed the gj/production-code-stripping branch from af60ace to 7362ad0 Compare November 12, 2016 17:49

GavinJoyce force-pushed the gj/production-code-stripping branch from 7362ad0 to 6e4cdbe Compare November 12, 2016 21:53

GavinJoyce mentioned this pull request Nov 12, 2016

modify the config GavinJoyce/babel-plugin-remove-functions#15

Merged

Ember CLI: Production code stripping

18dc017

GavinJoyce force-pushed the gj/production-code-stripping branch from 6e4cdbe to 18dc017 Compare November 12, 2016 22:33

GavinJoyce mentioned this pull request Nov 16, 2016

Support nested destructuring? GavinJoyce/babel-plugin-remove-functions#19

Open

cibernox mentioned this pull request Nov 29, 2016

RFC: JavaScript Modules API emberjs/rfcs#176

Merged

nathanhammond merged commit e896286 into ember-cli:master Dec 2, 2016

GavinJoyce deleted the gj/production-code-stripping branch December 2, 2016 07:54

This was referenced Jan 15, 2017

Added store.isFragment adopted-ember-addons/ember-data-model-fragments#235

Merged

[WIP] use the new store.isFragment method adopted-ember-addons/ember-data-factory-guy#268

Closed

rwjblue mentioned this pull request Apr 22, 2017

Add debug tooling babel plugins. emberjs/ember-cli-babel#133

Merged

rwjblue mentioned this pull request Jul 22, 2017

Correct place to report a bug? ember-codemods/ember-modules-codemod#23

Closed

chancancode mentioned this pull request Mar 6, 2018

[DOCS beta] debug functions are removed from prod build in apps emberjs/ember.js#16330

Merged


		TODO:

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


		# Unresolved questions

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

Ember CLI: Production code stripping #50

Ember CLI: Production code stripping #50

Conversation

GavinJoyce commented Apr 6, 2016 • edited Loading

webark commented Apr 6, 2016

GavinJoyce commented Apr 6, 2016

webark commented Apr 6, 2016 • edited Loading

GavinJoyce commented Apr 6, 2016 • edited Loading

GavinJoyce commented Apr 6, 2016

webark commented Apr 6, 2016 • edited Loading

rwjblue commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webark commented Apr 6, 2016

runspired commented Apr 6, 2016

GavinJoyce commented Apr 6, 2016

nathanhammond commented Apr 6, 2016

runspired commented Apr 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Apr 17, 2016

les2 commented Oct 30, 2016

stefanpenner commented Oct 31, 2016 • edited Loading

GavinJoyce commented Nov 2, 2016 • edited Loading

rwjblue commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixonic commented Nov 7, 2016

runspired commented Nov 7, 2016

Gaurav0 commented Nov 7, 2016

GavinJoyce commented Nov 7, 2016 • edited Loading

les2 Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinJoyce Nov 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinJoyce commented Nov 8, 2016 • edited Loading

GavinJoyce commented Nov 10, 2016 • edited Loading

rwjblue commented Nov 10, 2016

Choose a reason for hiding this comment

rwjblue Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanhammond commented Nov 11, 2016 • edited Loading

GavinJoyce commented Nov 12, 2016 • edited Loading

nathanhammond commented Nov 16, 2016 • edited Loading

nathanhammond commented Dec 2, 2016

GavinJoyce commented Apr 6, 2016 •

edited

Loading

webark commented Apr 6, 2016 •

edited

Loading

GavinJoyce commented Apr 6, 2016 •

edited

Loading

webark commented Apr 6, 2016 •

edited

Loading

stefanpenner commented Oct 31, 2016 •

edited

Loading

GavinJoyce commented Nov 2, 2016 •

edited

Loading

GavinJoyce commented Nov 7, 2016 •

edited

Loading

les2 Nov 8, 2016 •

edited

Loading

GavinJoyce Nov 12, 2016 •

edited

Loading

GavinJoyce commented Nov 8, 2016 •

edited

Loading

GavinJoyce commented Nov 10, 2016 •

edited

Loading

rwjblue Nov 11, 2016 •

edited

Loading

nathanhammond commented Nov 11, 2016 •

edited

Loading

GavinJoyce commented Nov 12, 2016 •

edited

Loading

nathanhammond commented Nov 16, 2016 •

edited

Loading