feat(metro-transformer): Add @rnx-kit/metro-transformer package and transformer plugins#4022
feat(metro-transformer): Add @rnx-kit/metro-transformer package and transformer plugins#4022
Conversation
…/jasonvmo/metro-transformer
…sformer merging and selecting babel transformers based on file type
…/jasonvmo/metro-transformer
tido64
left a comment
There was a problem hiding this comment.
I don't see the esbuild transformer that you've built so I'm going to assume it'll be in a separate PR. I have some questions about it:
- How does it preserve the source maps?
- I assume we're preprocessing source with esbuild, then pass the serialized content to upstream transformer. Are we still keeping the filename intact? Does that mean we're still using the Babel parser instead of Hermes?
The esbuild transformer PR will be dependent on this one. I'll likely publish it as a draft if nothing else today though it will include these changes until they are checked in. With regards to your questions:
In my first draft of the transformer I tried renaming the files as .js and then patching the sourcemaps. This breaks the codegen plugin though as it needs the .ts filename. |
| : undefined | ||
| ); | ||
| Object.assign(metroConfig.transformer, esbuildTransformerConfig); | ||
| // otherwise, add it to the list to be merged by the MetroTransformer |
| const transformers: ExtendedTransformerConfig[] = metroConfig.transformer | ||
| ? [metroConfig.transformer] | ||
| : []; |
There was a problem hiding this comment.
transformer is always set because Metro requires it. This also means that we will always run the merger logic below.
I tested this locally with only tree shaking enabled and this is what it returned:
transformOptions: {
transform: { experimentalImportSupport: false, inlineRequires: false },
customTransformerOptions: {
upstreamTransformerPath: '/~/node_modules/.store/@react-native-metro-babel-transformer-virtual-38655d5e06/package/src/index.js'
}
}Ideally, we don't want this plugin to be running at all if it's not set.
| // start with a hash of this file's contents as the cache key | ||
| const cacheKeyParts: (string | Buffer)[] = [fs.readFileSync(__filename)]; |
There was a problem hiding this comment.
Should this be moved inside getCacheKey? And can we avoid the fact that it's accessing disk on load, before it gets used at all?
| getCacheKey?: () => string; | ||
| }; | ||
|
|
||
| import type { BabelTransformerArgs as BaseTransformerArgs } from "metro-babel-transformer"; |
There was a problem hiding this comment.
Import statements should be grouped together.
| /** | ||
| * Get the cached cache key. Will be recalculated if additional parts are added to cacheKeyParts | ||
| */ | ||
| export const getCacheKey = (() => { |
There was a problem hiding this comment.
Should we be using metro-cache-key directly? I'm also not fond of using md5 here but it looks like this comes from upstream.
| filename: string, | ||
| babelTransformers: Record<string, string> | ||
| ): string | undefined { | ||
| for (const [pattern, transformerPath] of Object.entries(babelTransformers)) { |
There was a problem hiding this comment.
If this happens a lot in a loop (I assume it does), it's better to iterate over an actual Map than allocating a lot of arrays and thrashing the gc.
| let transformerPath = tryRequireResolve(baseTransformer, process.cwd()); | ||
| if (!transformerPath) { | ||
| // next try to get metro-config from the package root and resolve from there | ||
| const metroConfigPath = tryRequireResolve( | ||
| `@react-native/metro-config/package.json`, | ||
| process.cwd() | ||
| ); | ||
| if (metroConfigPath) { | ||
| transformerPath = tryRequireResolve( | ||
| baseTransformer, | ||
| path.dirname(metroConfigPath) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be inverted. The local copy should always be preferred. The one you'll find at root (if you're using node-modules linkage) is the most popular one, which is not guaranteed to be the same version being used for the current project.
| const { customTransformerOptions } = args.options | ||
| .customTransformOptions as unknown as { | ||
| customTransformerOptions: CustomTransformerOptions; | ||
| }; |
There was a problem hiding this comment.
Shouldn't we be using ExtendedTransformerConfig here?
| { | ||
| "extends": "@rnx-kit/tsconfig/tsconfig.node.json", | ||
| "compilerOptions": { | ||
| "rootDir": "src", |
There was a problem hiding this comment.
rootDir is not necessary if noEmit: true
| "rootDir": "src", |
Description
@rnx-kit/metro-transformer
The core part of this change is the addition of the
@rnx-kit/metro-transformerpackage. This package:getTransformerOptionsresults which by default overwrite one another.@rnx-kit/types-metro-config
This change also introduces the types only package
@rnx-kit/types-metro-configwhich contains definitions for serializers, transformers, and the general shape of rnx-kit plugins.Other changes
The transformer package is hooked into the bundle commands in the CLI by aggregating transformer configs. By default there is 1 config which comes from the metro config. If we have plugins that add additional transformer configs or if we apply the esbuild serializer transformer config the metro-transformer package will form them into a final transformer config.