feat(plugin-react): allow options.babel to be a function

[cyco130]

Description

This PR adds an ssrPlugins options to plugin-react that overrides the babel.plugins option for SSR because sometimes different transforms are needed in the browser than on the server.

UPDATE: We settled on allowing options.babel to be a function to solve the same problem more generally instead.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[aleclarson]

Any reason for making it ssrPlugins and not babel.ssrPlugins? And what about ssrPresets?

[aleclarson]

Alternatively, we could do an ssrOverrides option whose value is a Babel config object.

Unresolved questions

[cyco130]

Any reason for making it ssrPlugins and not babel.ssrPlugins? And what about ssrPresets?

Because babel was passed to Babel directly, this seemed less intrusive. But:

Alternatively, we could do an ssrOverrides option whose value is a Babel config object.

This is probably the way to go. I'll update my branch at the first opportunity.

[patak-dev]

@cyco130 couldn't this be done with conditional config? It doesn't seem scalable to add ssrXXX for each option.

[cyco130]

@patak-dev it would work for build but not in dev. The idea is to be able to use different transforms when transform(code, id, options) is called with options.ssr == true even in dev.

BTW I noticed the new plugin.api.reactBabel API requires some more thinking. I had the idea to allow opt.babel to be a function to be called with id and options to open up even more possibilities at the expense of recreating the config for every file. Would that make sense?

[aleclarson]

I had the idea to allow opt.babel to be a function to be called with id and options

I like this direction!

[cyco130]

@aleclarson Updated to implement that idea.

[cyco130]

Hmm, maybe it would be best to pass id and options to plugin.api.reactBabel too? The if (typeof opts.babel !== "function") optimization won't work then.

[aleclarson]

@patak-dev LGTM 🎸

[patak-dev]

This makes a lot more sense to me now. Isn't it too much to redo the babel plugins on each transform call? It seems inefficient. For the original use case, wouldn't a set of plugins that are computed once be enough? (like it was before being a function?).
I think we should properly justify having it on transform instead of being a one-off. Almost everything in Vite is configured once, and then filters (include/exclude).

[aleclarson]

Even before, Babel would resolve the plugins on every transformation, so the performance impact here is negligible.

For the original use case, wouldn't a set of plugins that are computed once be enough? (like it was before being a function?)

The Babel options object is cached in the staticBabelOptions variable if no api.reactBabel hooks exist and the babel plugin option is not a function, so the original use case works almost exactly like before, with the exception that now we can't safely reuse the Babel options if api.reactBabel hooks exist, because they might be using the new ssr and/or file properties exposed to them.

I think we should properly justify having it on transform instead of being a one-off. Almost everything in Vite is configured once, and then filters (include/exclude).

I'd say it's ideal to allow per-file configuration whenever possible, since it provides more flexibility. For example, higher level frameworks built on Vite might need to apply certain plugins based on file structure or in SSR only. It would hurt performance to implement their own Babel transformer.

[patak-dev]

We got another +1 to merge this in today's team meeting. @cyco130 we can move forward once the tests are green 👍🏼

[cyco130]

I've fixed the related test but I don't quite understand the (probably unrelated) failing IIFE tests on MacOS. I'd appreciate some help with it :)

[patak-dev]

I've fixed the related test but I don't quite understand the (probably unrelated) failing IIFE tests on MacOS. I'd appreciate some help with it :)

That was a glitch in Vite's CI, sorry about that

[aleclarson]

Another option is to use Object.defineProperties to define ssr and file options with enumerable: false (note this is the default when using defineProperties). That will prevent Babel from getting them, since non-enumerable properties are ignored by the spread operator.

[cyco130]

I feel the intention would be less obvious and require a comment. But I can do that if you feel it'd be better.

[patak-dev]

Why are these in the same object? I think the API would be more clear with three args. ssr and file are quite different than the rest of the object

type ReactBabelHook = (
  babelConfig: ReactBabelOptions,
  options: { ssr, file }, // or context?
  config: ResolvedConfig
) => void

[aleclarson]

ok let's do that, and I vote context as the name

[cyco130]

Done.

[patak-dev]

Thanks for the changes @cyco130!

Sorry, one last question 👀
Why is it called file? Now that it is in the second parameter, should we rename it to id? file isn't that good in case there are virtual modules, no?

[cyco130]

Fixed (wasn't my idea anyway :P )