Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM TypeScript support in the metro-resolver #886

Open
MaijaHeiskanen opened this issue Nov 1, 2022 · 3 comments
Open

ESM TypeScript support in the metro-resolver #886

MaijaHeiskanen opened this issue Nov 1, 2022 · 3 comments
Assignees

Comments

@MaijaHeiskanen
Copy link

MaijaHeiskanen commented Nov 1, 2022

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

metro-resolver cannot resolve TypeScript imports that are in ESM format, e.g. have .js file extension in the import. It only adds extensions listed in sourceExts at the path, but does not consider situations where the original file extensions needs to be replaced.

What is the expected behavior?

metro-resolver should try to resolve TypeScript imports that are in ESM format. metro-resolver should try out the different file-extensions as it currently does, but in addition try out the file extensions also after removing the original file extension of the import. Example:

import {thing} from './something.js

Currently metro-resolver by default tries to resolve this as ./something.js, ./something.js.native, ./something.js.android.js, ./something.js.ts, ./something.js.android.ts, ... etc.

It should also try to resolve it as following: Currently metro-resolver by default tries to resolve this as ./something, ./something.native, ./something.android.js, ./something.ts, ./something.android.ts, ... etc. For my use case it should try to replace .js with .ts and .tsx, but there might be other use cases too.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

Metro version: 0.67.0
Metro-resolver version: 0.67.0
Node version: 16.18.0
Yarn version: 1.22.4
Windows 11 and MacOS

// metro.config.js
const path = require('path');
const {getDefaultConfig} = require('metro-config');

module.exports = (async () => {
    const {
        resolver: { sourceExts, assetExts }
    } = await getDefaultConfig();

    return {
        projectRoot: path.resolve(__dirname, '.'),
        // watchFolders: [path.resolve(__dirname, './node_modules')],
        watchFolders: [
            path.resolve(__dirname, '../../node_modules'),
            path.resolve(__dirname, '../../packages/package1'),
            path.resolve(__dirname, '../../packages/package2'),
            path.resolve(__dirname, '../../packages/package3')
        ],
        resolver: {
            assetExts: assetExts.filter((ext) => ext !== 'svg'),
            // https://github.com/facebook/metro/issues/1#issuecomment-453450709
            extraNodeModules: new Proxy(
                {},
                {
                    get: (target, name) => path.join(process.cwd(), `node_modules/${name}`)
                }
            ),
            sourceExts: [...sourceExts, 'svg']
        },
        transformer: {
            babelTransformerPath: require.resolve('react-native-svg-transformer'),
            // eslint-disable-next-line require-await
            getTransformOptions: async () => ({
                transform: {
                    experimentalImportSupport: false,
                    inlineRequires: false
                }
            })
        }
    };
})();
@huntie huntie self-assigned this Nov 5, 2022
@huntie
Copy link
Member

huntie commented Nov 5, 2022

Thanks for bringing this to our attention!

If my understanding is right, you're trying to enable this new TypeScript ES module compiler behaviour in your app's source code.

Put shortly, the behaviour you suggest above is functionality we don't yet support and would be independent from Metro's current concepts (so yes, new feature 🙂 — adding detail here to provide a clear spec).

Current module extension concepts

Replacing explicitly-provided extensions (e.g. ./something.js -> ./something.ts) has until now not been a necessary feature:

  • metro-resolver is intended to support multiple sourceExts and platform-specific extensions only when resolving an extensionless import (where the file name is given only) (this can independently contain a .). Extensions derived from sourceExts and platforms are appended.
  • Extensioned imports are an opt-out from this, which we have formalised recently in supporting spec-compliant ES modules (cjs/mjs): c1c6d9c.

Under the above, file.js -> file.ios.js is not intended behaviour.

TypeScript ES module imports

TypeScript appears to be handling ES module extensions differently — requiring metro-resolver to resolve a source file which isn't present on the filesystem (e.g. .cjs import referencing a .cts file).

// @filename: helper.cts
export function helper() {
    console.log("hello world!");
}
 
// @filename: index.mts
import foo from "./helper.cjs"; // <- replaced with .cts

So, we'll need some new config concept or builtin behaviour in metro-resolver to handle this. Perhaps, .cts and .mts as "extra" extensions that are swappable for specific fully-specified extensions. 📥 This will go in our task backlog!

Recommendations

  • Until we add support for TypeScript ES modules, we advise not to enable this compiler option yet in a React Native project — please continue to use CommonJS.
  • In theory, you should be able to use either resolveRequest or redirectModulePath to customise this — a workaround rather than a recommendation.

@huntie
Copy link
Member

huntie commented Nov 8, 2022

Some more digging in the TypeScript proposal on why this design was decided:

Because TypeScript never rewrites module specifiers in its JavaScript emit, the only possible answer is that it should mirror whatever resolution behavior the code’s intended runtime module resolver has
— microsoft/TypeScript#50152

There is also feedback/concern about this: Concerns with TypeScript 4.5's Node 12+ ESM Support. And this reply implies the TS team is committed to this design, and CommonJS also remains the default recommended setup from their point of view (intentional move of publishing no migration guide).

From the React Native side:

  • We should wait until resolution of all raised concerns on the TypeScript side before implementing support for TypeScript ESM extension resolution by default in Metro.
    • One point of alignment/change might be that import specifiers must point to source files, since these are the modules that React Native will resolve at dev/build time.
  • We will continue to recommend CommonJS (and modern syntax + features via Babel) as the supported setup for React Native app developers.

@mo22
Copy link

mo22 commented Jan 23, 2023

can be kind-of fixed with the following code in metro.config.js:

const path = require('path');
const fs = require('fs');

/** @type {import('metro-config').ConfigT} */
module.exports = {
  // [...]
  resolver: {
    resolveRequest: (context, moduleName, platform) => {
      if ((moduleName.startsWith('.') || moduleName.startsWith('/')) && moduleName.endsWith('.js')) {
        const possibleResolvedTsFile = path.resolve(context.originModulePath, '..', moduleName).replace(/\.js$/, '.ts');
        if (fs.existsSync(possibleResolvedTsFile)) {
          return {
            filePath: possibleResolvedTsFile,
            type: 'sourceFile',
          };
        }
      }
      return context.resolveRequest(context, moduleName, platform);
    },
  }
  // [...]
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants