Ember CLI: Production code stripping

GavinJoyce commented 8 years ago

webark commented 8 years ago

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 commented 8 years ago

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 https://github.com/ember-cli/rfcs/pull/50#issuecomment-206460703

webark commented 8 years ago

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 commented 8 years ago

Oh. In the documentation it sates

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

GavinJoyce commented 8 years ago

.. 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 commented 8 years ago

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 commented 8 years ago

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

webark commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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

les2 commented 8 years ago

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 commented 8 years ago

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

GavinJoyce commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

It looks like there are three items of feedback to be addressed here @GavinJoyce

[x] Adding a comment about the breaking change to arguments with side-effects. Perhaps this stripping can be disabled. Regardless we should call it out in the blog post.
[x] ~~API design for the test helpers to be exported (expectAssertion / expectWarn / expectDeprecation)~~ Core team nuked this request. Let's decouple it, but please let's not add any helpers without design.
[x] How does this work with addons? (Basically, be sure to say that is works when the addon is enabled and not when it doesn't. And that if Ember-CLI apps will have this addon by default, addons will also have it by default.)

Excited to see this move forward though, it would be a good addition to the default ember-cli addon stack.

GavinJoyce commented 8 years ago

Thanks for the feedback, I've updated the RFC.

mixonic commented 8 years ago

Great, shipping this to FCP today!

runspired commented 8 years ago

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 commented 8 years ago

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

GavinJoyce commented 8 years ago

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

GavinJoyce commented 8 years ago

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

GavinJoyce commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

[x] Squash, rebase.
[x] Note that it must account for destructured and reassigned invocations of these functions.
[x] Stated intent to support both Babel 5/Babel 6.

GavinJoyce commented 7 years ago

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

nathanhammond commented 7 years ago

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 commented 7 years ago

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!

ember-cli / rfcs

Ember CLI: Production code stripping #50