Babel config invalidation

[padmaia]

↪️ Pull Request

This PR updates @parcel/transformer-babel to use the new config invalidation pattern. It also uses @babel/core to resolve babel configs, so babel.config.js is now supported. There are certain newly supported scenarios that are non optimal (such as having a babel.config.js file in the first place) in which case a warning will be administered. One more notable change is that it is now using core-js over the deprecated @babel/polyfill.

✔️ PR Todo

• Implement config.shouldInvalidateOnStartup() now that graphs are being cached
• Figure out how to surface performance warnings (can we deduplicate?)
• Integration tests
• Add a README that describes how the plugin works and steps to take to optimize for caching
• Separate Config into public and internal classes

[devongovett]

Looking good! I left a few inline comments and questions. Would like to nail down the public API a bit more. Happy to discuss in person/on a call if that's easier.

[devongovett]

Did you consider an API where you the result directly from the plugin, instead of mutating the passed object? We could also automatically compute the hash from the result in core, unless there's a case where leaving that to the plugin is required.

loadConfig({config}) {
  return {
    filePath: '...', // instead of setResolvedPath
    config: {...}    // instead of setResult
  };
}

I'm ok with either, just wanted to ensure we considered it.

[padmaia]

Yeah, that's something I've discussed with @wbinnssmith a bit. One reason I was leaning this way is that it is more consistent with how we pass Asset to the other methods. It also alleviates some work with methods like getConfig that both help with resolving/parsing config and also adding whichever file was resolve to included files.

We could also automatically compute the hash from the result in core, unless there's a case where leaving that to the plugin is required.

Yeah, my thinking was it would be nice to be flexible because it's possible a resolved config might contain more information than is relevant for invalidation. One example where we're actually using it right now is the parcel config. For invalidation of a pipeline transformation, we only care about one pipeline, not the entire config. We could accomplish the same thing in other ways for Parcel, but it seemed like it might be useful for plugins. For instance, plugins might be using information from package.json, but they only care a single field. (That reminds me the babel transformer should probably invalidate if the source field changes)

[devongovett]

With transformers we kinda actually support both: there is that TransformerResult type which has fields like code and map that you can return instead of mutating the asset. We found this to be challenging to deal with when eg adding dependencies, and passing assets into util functions, and mutation was easier there. The difference here is that Assets start out fully hydrated (they have input code etc.), whereas config starts out as an empty object. It just felt a little strange. I'm not totally against what's here, just curious what it would look like another way.

I think your usecase with hashes is a good one to allow control to plugins, but I think we should make it optional. i.e. if no hash is set by the plugin, we could set one automatically based on the result. Perhaps the shouldReload setting could also set the hash to something random?

[padmaia]

I haven't had the time to explore this yet. Is this a blocker for the PR or could we follow up on it?

[devongovett]

So, if I understand correctly, all config for all plugins is loaded in the main thread. Then it's sent back to the transformer thread, and rehydrated. But, if it needs to be reloaded, it is immediately reloaded again on the transformer thread. So we end up needing to load it twice in that case?

[padmaia]

Yeah, for babel that will happen if you are using a babel.config.js file and you have something like:

module.exports = {plugins: [require('babel-plugin-whatever')]}

because at that point all we have is a function which can not be rehydrated. And unfortunately, we don't know if the user is using this type of configuration until we try to load it. Not ideal obviously, but I thought it would be better to just support it and surface a warning that this type of configuration has performance impacts. I was thinking it would also be a good idea to document how to optimize for caching in a README for this package. I'll add that as a TODO.

[devongovett]

Do configs need to be loaded on the main thread at all? I guess to check the graph? What if we just didn't store anything in the graph at all for configs that are uncacheable?

[padmaia]

We probably need some placeholder so we can at least invalidate on startup. Might not have to store an entire Config class though. Is this something we can follow up on?

[wbinnssmith]

Really excited about this 😃

I agree with @devongovett that we should have a stricter public interface for Config that calls through to internals.

[padmaia]

@devongovett I've separated Config into a public and internal class. Going to resolve conversations related that so it's easier for me to tell what's still left to do, but let me know what you think.

[devongovett]

How do we feel about changing this to useBuiltIns: 'usage'? That would let babel automatically add only the polyfills needed from babel-polyfill instead of requiring the user to import 'babel-polyfill' in their app. See #1762.