embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
337 stars 137 forks source link

Disable `babel-loader` cache when there are non-serializable plugins #723

Open charlespierce opened 3 years ago

charlespierce commented 3 years ago

Currently, babel-loader always has the cacheDirectory set here. However, when there are non-serializable babel plugins in the app, the _babel_config_.js file that is used for the babel settings contains a nonce value. Since babel-loader uses (in part) the babel config to build the cache key, that key always changes between builds, meaning the cache is never used.

Since we know ahead of time if there are non-serializable addons, we should disable the babel-loader cache (by setting cacheDirectory: false) in those situations, as it is always a net negative for the build.

ef4 commented 3 years ago

I think the bug here is that the nonce is unstable. It shouldn't change when the underlying non-serializaable object in the babel config is still === the previous one, and that in turn should be true or it's a bug.

charlespierce commented 3 years ago

What does it mean in this case for two non-serializable objects to be equal in two separate builds? It's possible that the cacheDirectory property is useful for an ember server environment, since that will all the be the same process. However, in my tests on a moderately large project, enabling the cache was causing builds to take ~50% longer total, and that was not mitigated in subsequent builds without any code changes.

ef4 commented 3 years ago

I was referring to a rebuild within the same process. The cache is supposed to assist in that case too.

But yes, I see your point that caching after a process exits is pointless.

Maybe we should make this an option into @embroider/webpack. If the cache overhead is this bad for your use case, would you even want the cache enabled for the serializable case?

rwjblue commented 3 years ago

If the cache overhead is this bad for your use case, would you even want the cache enabled for the serializable case?

I guess it depends how much it would assist if the cross process caching did work (which is hard for us to test at the moment).

ef4 commented 3 years ago

Is pluginHints not able to work around your current non-serializable things?

As a meta-point: while I'm resistant to adding new options to the general-purpose stage3 interface that all stage3 packages must understand, it's much less of a big deal to add options directly to @embroider/webpack. So for stuff like this where @embroider/webpack is managing config for you, it's fine to solve this kind of problem by adding another option.

charlespierce commented 3 years ago

I'll investigate the pluginHints to see if that will work to allow us to at least test. But I also think that giving user some control over that setting would be useful, as there are many factors that can impact the usefulness of caching, most of which can only be observed by trying it out.