Fix in 4.16.3 broke use of certain libraries when allowJs is true

lawrenceong commented 3 months ago

Acknowledgements

[X] I read the documentation and searched existing issues to avoid duplicates
[X] I understand this is a bug tracker and anything other than a proven bug will be closed
[X] I understand this is a free project and relies on community contributions
[X] I read and understood the Contribution guide

Minimal reproduction URL

https://stackblitz.com/edit/node-epykqv

Problem & expected behavior (under 200 words)

From and including version 4.16.3 of tsx, the bug caused the following problem:

If tsconfig.json contains a compiler option of allowJs set to true:

{
  "compilerOptions": {
    "allowJs": true
  }
}

Then certain packages will fail when used with tsx. From the minimal reproduction URL, using libphonenumber-js and tsx, a simple call will result in the following error:

> tsx index.ts

Error: [libphonenumber-js] `metadata` argument was passed but it's not a valid metadata. Must be an object having `.countries` child object property. Got an object of shape: { default }.
    at validateMetadata (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/metadata.js:591:11)
    at new Metadata (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/metadata.js:43:5)
    at parse (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/parse.js:89:14)
    at parsePhoneNumberWithError (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/parsePhoneNumberWithError_.js:19:32)
    at parsePhoneNumber (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/parsePhoneNumber_.js:32:55)
    at parsePhoneNumber (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/build/parsePhoneNumber.js:20:44)
    at call (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/min/index.cjs.js:14:14)
    at parsePhoneNumberFromString (/home/projects/node-epykqv/node_modules/.pnpm/libphonenumber-js@1.11.7/node_modules/libphonenumber-js/min/index.cjs.js:18:9)
    at eval (/home/projects/node-epykqv/index.ts:2:864)
    at Object.eval (/home/projects/node-epykqv/index.ts:3:3)

index.ts:

import parsePhoneNumber from 'libphonenumber-js';

console.log(parsePhoneNumber('+61491570006').isValid());

Bugs are expected to be fixed by those affected by it

[ ] I'm interested in working on this issue

Compensating engineering work will speed up resolution and support the project

[x] I'm willing to offer $10 for financial support

paul-b-manning commented 3 months ago

I am seeing this same issue. I discovered this after recently upgrading to TSX v4.17 from an earlier version.

At line 10 in libphonenumber-js/index.cjs.js they load a json file using CommonJS require syntax. This ends up with an object with the default property set to an accessor to the json object instead of the json object itself.

The line is: var metadata = require('./metadata.min.json')

For now, I have to rollback to TSX v4.16.2.

privatenumber commented 3 months ago

Thanks for the issue and minimal reproduction.

Looks like the TS resolver is trying the .js extension to handle the implicit extension case, and it happened to match, which ends up loading the wrong file.

Will try to look into a fix later this week.

If anyone is interested in helping, feel free to contribute constructively by identifying the problem and proposing solutions.

wrose504 commented 1 week ago

While debugging this today, I found a difference between 4.16.2 and 4.16.3 in how the libphonenumber-js/min/index.cjs was processed. In particular, the var metadata = require('../metadata.min.json') sets metadata to the JSON in 4.16.2, but in 4.16.3, it sets it to {default: { /* the JSON */, "__esModule", { value: true } }. That then means that metadata.countries is not defined, because it is at metadata.default.countries instead.

I was able to work around this in my code by using the libphonenumber-js/core import and separately importing the minimal metadata:

import {parsePhoneNumber} from 'libphonenumber-js/core';
import minimalMetadata from 'libphonenumber-js/min/metadata';

console.log(parsePhoneNumber('+61491570006', minimalMetadata).isValid());

This could be because libphonenumber-js/min/metadata maps to a JS version of the JSON in package.json:

{
  /* snip */
  "exports": {
    /* snip */
    "./metadata.min": {
      "import": "./metadata.min.json.js",
      "require": "./metadata.min.json"
    },
  }
}

I haven't dug into this further as I don't use much from libphonenumber-js and the workaround seemed easy enough.

wrose504 commented 1 week ago

I dug a little further and I think that mapTsExtensions should try the originally requested path before trying variants where it adds extensions.

diff --git a/src/utils/map-ts-extensions.ts b/src/utils/map-ts-extensions.ts
index 34f3a68..6af9e3e 100644
--- a/src/utils/map-ts-extensions.ts
+++ b/src/utils/map-ts-extensions.ts
@@ -47,6 +47,9 @@ export const mapTsExtensions = (
                );
        }

+       // If we're guessing extensions, and the file _has_ an extension, start with it.
+       if (!tryExtensions && extension) tryPaths.push(filePath);
+
        const guessExtensions = (
                (
                        !(filePath.startsWith(fileUrlPrefix) || isFilePath(pathname))

Currently when resolving ./path/to/mymodule.js, mapTsExtensions will recognize the .js extension and try replacing it with .ts, .tsx, .js (i.e. here it tries the original request), then .jsx, and then it will try suffixing so that .js becomes .js.ts, .js.tsx, .js.js, and .js.jsx.

When resolving ./path/to/mydata.json, mapTsExtensions does not recognize the .json extension and goes straight to suffixing, so it tries .json.ts, .json.tsx, .json.js, and .json.jsx. Unlike previously, it never actually tries .json, and only tries it after .json.js. Since libphonenumber-js has .json.js files, these are what get imported, presumably then as ES modules with a default export, instead of just JSON as expected.

If I add in the line above to try the original file if it has an extension, then the .json is tried before all the suffixed options, and resolves correctly.

This might also explain a pretty significant slowdown I am seeing in how quickly tsx is able to load a large project: if it's having to stat a bunch more options before giving up, it's probably wasting a lot of time.

privatenumber commented 1 week ago

To resolve this, tsx will need a custom CJS resolver. The native CJS resolver has limitations, especially in extending the export map resolution behavior, which prevents a fix with the current setup. Additionally, there's a problematic discrepancy between tsx's CJS and ESM resolvers.

Creating a custom CJS resolver could also address other problems where passing in conditions is necessary.

One potential solution I'm considering—but haven't yet explored—is switching to the Oxc resolver.

wrose504 commented 1 week ago

I don't know the intricacies of module resolution, but it seemed like things were okay with 4.16.2. Were there specific use cases you were trying to fix during the 4.16.3 refactor? Would it be a backward step to try to change the logic like I suggested?

I was thinking I could make a small PR with the change above, but if it doesn't actually make sense I am ok to wait for an alternative solution based on Oxc.

privatenumber / tsx