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

Uncovered cases explored from a project migration #123

Open
timofei-iatsenko opened this issue Feb 1, 2025 · 4 comments
Open

Uncovered cases explored from a project migration #123

timofei-iatsenko opened this issue Feb 1, 2025 · 4 comments

Comments

@timofei-iatsenko
Copy link

Hey there! First off, thanks for this awesome tool—it's been a huge time-saver! 🙌

I recently migrated a TypeScript project to ESM and used ts2esm to correctly add .js extensions in imports. While it worked well overall, I ran into a few challenges that I think could improve the tool even further.

1️⃣ Partial Support for tsconfig Paths

I saw that tsconfig path support works to some extent, but I had issues in an Nx monorepo with /apps/* and /libs/*. These are linked using tsconfig paths, and ts2esm only resolved the imports correctly after I moved libs inside an apps folder. My workaround looked like this:

  1. Move libs/* into apps/myapp/libs/*
  2. Update tsconfig.json paths accordingly
  3. Run ts2esm
  4. Move libs back to their original location
  5. Repeat for each app

If ts2esm could resolve paths of aliases outside the current project dir, that would be amazing!

2️⃣ Handling Package Exports and Subpath Imports

My project had a mix of modern packages (with package.json exports) and older ones using subpath imports. It would be great if ts2esm could leverage TypeScript's built-in resolver to automatically handle these cases.

Example 1: Legacy Package (lodash)

lodash doesn't have an exports field, so it requires .js in the import path:

// Before (CommonJS)
import omit from 'lodash/omit';

// After (ESM)
import omit from 'lodash/omit.js';

Currently, ts2esm doesn’t handle this, so I had to fix it manually.

Example 2: Modern Package (firebase-functions)

firebase-functions has an exports field, so no changes are needed:

// This works in both CJS and ESM
import { HttpsError } from 'firebase-functions/v1/https';

However, in cases where an import incorrectly targets a private subpath, it breaks in ESM:

// This works in CJS but fails in ESM
import type { ObjectMetadata } from 'firebase-functions/lib/v1/providers/storage';

Since TypeScript correctly flags these issues, maybe ts2esm could warn or ignore them?

3️⃣ Dynamic Imports Are Not Converted

I also noticed that dynamic imports aren’t updated with .js extensions:

const module = await import('./my-function.function'); 
// Expected:
const module = await import('./my-function.function.js'); 

Handling this would make ts2esm even more powerful!

Would love to hear your thoughts! Let me know if I can help test anything. 😊

@bennycode
Copy link
Owner

bennycode commented Feb 3, 2025

Hey @timofei-iatsenko, thanks a lot for putting in the effort to write such a detailed report! 🙏 Seeing others engage so deeply with projects I maintain gives me a real boost of motivation to keep working on them. 💪

Let's go through the topics...

1️⃣ Partial Support for tsconfig Paths

Is this the structure of your Nx Monorepo?

  • apps/myapp/tsconfig.json
  • apps/myapp/apps/*
  • apps/myapp/libs/*

Do you have a single main tsconfig.json, or are there multiple? I ask because I use sourceFile.getProject() from ts-morph](https://github.com/dsherret/ts-morph) to locate TS configs. @fgarcia mentioned that sourceFile should be a .d.ts file to properly find references from co-located packages: dsherret/ts-morph#1242 (comment)

2️⃣ Handling Package Exports and Subpath Imports

Right now, ts2esm simply checks if a JS file is inside the node_modules directory of the referenced package. It doesn’t parse the package.json file of installed modules at all. It sounds like supporting this would require reading the exports field from package.json. Might take some effort to implement.

Relevant docs: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#package.json-exports-imports-and-self-referencing

Currently, my isNodeModuleRoot can already locate the relevant package.json file. That would be a good entrypoint to build the parsing functionality. 💡

3️⃣ Dynamic Imports Are Not Converted

Great catch! I tested the following code in the TypeScript AST Viewer:

const module = await import('./my-function.function'); 

It shows that this call expression is of type SyntaxKind.ImportKeyword, which isn't currently handled in convertFile.

This snippet could help detect it:

sourceFile.getVariableStatements().forEach(statement => {
  const declaration = statement.getDeclarations()[0];
  const initializer = declaration?.getInitializerIfKind(SyntaxKind.CallExpression);
  const kind = initializer?.getExpression().getKind();
  if (kind === SyntaxKind.ImportKeyword) {
    // ...
  }
});

It should be relatively easy to implement. I'll see when I can get to it. 🫣

Thanks again for your report!

Best from Berlin,
Benny

@timofei-iatsenko
Copy link
Author

Is this the structure of your Nx Monorepo?

  • apps/myapp/tsconfig.json
  • apps/myapp/apps/*
  • apps/myapp/libs/*
  • /tsconfig.json - root tsconfig, has path mappings such as @mylib - > /libs/mylib/src/index.ts
  • /libs/*
  • /libs/mylib/tsconfig.json lib tsconfig extends root one and has an include field for src/*
  • /libs/mylib/src/*
  • /apps/myapp/tsconfig.json app tsconfig extends root one and has an include field for src/*
  • /apps/myapp/src/*

I started the script in each project in monorepo one by one eq: cd ./apps/myapp && npx ts2esm

Right now, ts2esm simply checks if a JS file is inside the node_modules directory of the referenced package. It doesn’t parse the package.json file of installed modules at all. It sounds like supporting this would require reading the exports field from package.json. Might take some effort to implement.

I'm thinking might be utilizing real typescript resolver would help here?

So the logic might be like that:

  • take a current bare id, say firebase-functions/v1/https, try to resolve it with typescript's ts.resolveModuleName (see example here https://github.com/nrwl/nx/blob/master/packages/jest/plugins/resolver.ts#L69-L74)
  • if it resolved with moduleResolution: NodeNext + type: module, means this external import is ok. left it as is
  • If it's not resolved, add .js and try again. If finally you get result, replace import in source file.

@bennycode
Copy link
Owner

Thanks for your insights @timofei-iatsenko!

I implemented support for dynamic imports (#130) and I created myself a test case for "2️⃣ Handling Package Exports and Subpath Imports":

import omit from 'lodash/omit';
import {HttpsError} from 'firebase-functions/v1/https';

const object = {a: 1, b: '2', c: 3};
omit(object, ['a', 'c']);

export function logError() {
  console.log(HttpsError);
}

Using the current version of ts2esm it gets converted to this:

import omit from 'lodash/omit.js';
import {HttpsError} from 'firebase-functions/v1/https';

const object = {a: 1, b: '2', c: 3};
omit(object, ['a', 'c']);

export function logError() {
  console.log(HttpsError);
}
  • A .js extension is assigned to a "Legacy Package"
  • Imports of a "Modern Package" are untouched

Isn't that exactly what you wanted?

I also tried to use the built-in TypeScript resolver using this utility method:

import { SourceFile, ts } from 'ts-morph';

export const ProjectUtil = {
  getModule: (moduleName: string, sourceFile: SourceFile) => {
    const compilerOptions = sourceFile.getProject().getCompilerOptions();
    const host = ts.createCompilerHost(compilerOptions, true);
    return ts.resolveModuleName(
      moduleName,
      sourceFile.getFilePath(),
      sourceFile.getProject().getCompilerOptions(),
      host
    );
  },
};

However, it returns a resolution to the .d.ts file when asking it for 'lodash/omit':

{
  resolvedModule: {
    resolvedFileName: '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/@types/lodash/omit.d.ts',
    originalPath: undefined,
    extension: '.d.ts',
    isExternalLibraryImport: true,
    packageId: {
      name: '@types/lodash',
      subModuleName: 'omit.d.ts',
      version: '4.17.15',
      peerDependencies: undefined
    },
    resolvedUsingTsExtension: false
  },
  failedLookupLocations: [
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit/index.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit/index.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/lodash/omit/index.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/@types/lodash/omit/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/@types/lodash/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/@types/lodash/omit.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/src/node_modules/@types/lodash/omit/index.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js/index.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js/index.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/lodash.js/index.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/index.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/index.tsx',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/omit/index.d.ts',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/@types/lodash/omit/package.json'
  ],
  affectingLocations: [
    '/Users/bennycode/dev/bennycode/ts-node-starter/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/lodash/package.json',
    '/Users/bennycode/dev/bennycode/ts-node-starter/node_modules/@types/lodash/package.json'
  ],
  resolutionDiagnostics: undefined,
  alternateResult: undefined
}

I am not yet sure how I can use this information to decide if I should add a .js extension to 'lodash/omit' or not. 🤔

@timofei-iatsenko
Copy link
Author

Isn't that exactly what you wanted?

Looks awesome!

I also tried to use the built-in TypeScript resolver using this utility method:

That's a bummer, hoped it will free you from implementing this logic yourself.

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

2 participants