Ravenstine / ember-custom-elements

The easiest way to render parts of your Ember app using custom elements.
MIT License
15 stars 4 forks source link

Not compatible with production builds out-of-the-box #9

Closed alexlafroscia closed 4 years ago

alexlafroscia commented 4 years ago

From what I can tell, the default behavior does not work with Production builds of Ember.

The sigil that's looked for in the transformed files (~~EMBER~CUSTOM~ELEMENT~~) is stripped out of the compiled assets. I am assuming that this is the uglifier at work, which would make sense; the string appears to be unused, so it is just removed entirely from the payload.

Setting the deoptimizeModuleEval flag does not work, as then every component is expected to have a Custom Element configuration, which results in hundreds of lines like this

WARN: WARNING: ember-custom-elements: Custom element expected for `...` but none found.
alexlafroscia commented 4 years ago

I'm curious -- is it really necessary to have the whole Babel plugin present in order to opt out a bit of work?

The addon is already iterating over every route/component/etc. to do something, so the O(N) work happens no matter what. The presence of the sigil allows for the addon to avoid running the getCustomElements helper, but that honestly doesn't really feel expensive as it's just doing a couple of property checks.

Is there more to the sigil than just that that I'm not realizing? It seems like the overhead of running the Babel plugin and needing to introspect the actual source code of each module at run-time might not be worth the savings there...

alexlafroscia commented 4 years ago

I was able to get things working for our application with the deoptimizeModuleEval setting after all.

In our tests, unfortunately, we get the WARNING skew that I mentioned earlier. Thankfully, the production builds silence those warnings, so it's not annoying in that case at least.

I tried out a build of ember-custom-elements that removes the sigil altogether

https://github.com/alexlafroscia/ember-custom-elements/commit/2f346969f9c70eb03b3940773e79c91b37d045fc

This ended up tanking our test suite, due to every module getting evaluated eagerly by the instance initializer. I haven't exactly worked out why that was a problem, but 🤷‍♂️ it was!

Ravenstine commented 4 years ago

@alexlafroscia I'm sorry that you've encountered these issues. Admittedly, I haven't yet used my own add-on in production mode, and it makes sense that the build would strip out bare strings during uglification.

I believe I may have addressed both of your issues in this PR:

https://github.com/Ravenstine/ember-custom-elements/pull/10

I've ditched the Babel plugin and instead evaluate modules based on whether they import the add-on. I also made some changes to make sure that the decorator can be used in an add-on, and added a test for that scenario.

If you would like to, I could use your help verifying whether my PR resolves these issues. In the meantime, I'll be addressing a problem with a few tests in Travis, so I won't be merging the PR until that's fixed.

alexlafroscia commented 4 years ago

That sounds good @Ravenstine! I can definitely try your PR branch in our app with a production build and let you know if it still works.

Just so you know, I'm no longer trying to decorate any addon components; I refactored our codebase so that that's not necessary anymore for unrelated reasons to the issue I ran into here.

I did run into another issue with production builds, with regard to determining if a component is a Glimmer component

https://github.com/Ravenstine/ember-custom-elements/blob/aa2be1e46799e3438ba55628fae8c5b2e84c6d0b/addon/lib/common.js#L110

That line specifically looking at the name of a class does not work in a minified/uglified build; the name is stripped out and becomes whatever the uglifier turns the name of the function into. In my case, the name of the Glimmer base component class is literally n.

I will open a PR for an approach that I think will also work without needing the constructor name preserved.