Skip to content

Commit

Permalink
fix(tsImport): import module from commonjs
Browse files Browse the repository at this point in the history
  • Loading branch information
@privatenumber
privatenumber committed May 10, 2024
1 parent d6a5f05 commit 48f0a75
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/esm/hook/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const tsExtensionsPattern = /\.([cm]?ts|[tj]sx)($|\?)/;
export const isJsonPattern = /\.json(?:$|\?)/;

const getFormatFromExtension = (fileUrl: string): ModuleFormat | undefined => {
const extension = path.extname(fileUrl);
const extension = path.extname(fileUrl.split('?')[0]);

if (extension === '.json') {
return 'json';
Expand Down
2 changes: 1 addition & 1 deletion src/utils/transform/get-esbuild-options.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const patchOptions = (
const originalSourcefile = options.sourcefile;

if (originalSourcefile) {
const extension = path.extname(originalSourcefile);
const extension = path.extname(originalSourcefile.split('?')[0]);

if (extension) {
// https://github.com/evanw/esbuild/issues/1932
Expand Down
20 changes: 20 additions & 0 deletions tests/specs/api.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,26 @@ export default testSuite(({ describe }, node: NodeApis) => {
expect(stdout).toBe('Fails as expected 1\nfoo bar\nfoo bar\nFails as expected 2');
});

test('mts from commonjs', async () => {
await using fixture = await createFixture({
'import.cjs': `
const { tsImport } = require(${JSON.stringify(tsxEsmApiCjsPath)});
(async () => {
const { message } = await tsImport('./file.mts', __filename);
console.log(message);
})();
`,
'file.mts': 'export const message = "foo bar"',
});

const { stdout } = await execaNode(fixture.getPath('import.cjs'), [], {
nodePath: node.path,
nodeOptions: [],
});
expect(stdout).toBe('foo bar');
});

This comment has been minimized.

Sign in to view

Copy link
@privatenumber

privatenumber May 10, 2024

Author Owner

@fasttime

RE: eslint/rfcs#117 (comment)

Sorry, I prefer not to discuss in that thread since debugging tsx is off-topic (pings everyone & pollutes conversation).

There was a bug here but it's fixed now—you should be able to import a (TypeScript) module file from a CommonJS file.

This comment has been minimized.

Sign in to view

Copy link
@fasttime

fasttime May 11, 2024

Thanks for the ping @privatenumber! The issue I was facing is actually a different one. I was assuming that one could simply use tsImport() to import a .ts module written with ESM syntax, but apparently that's not possible without registering the CJS extension.

I can't create a repro on StackBlitz because it uses Node.js 18.18.0, but here is a gist: https://gist.github.com/fasttime/77abbb1671d68f950a5156ffcf994bdb. As you can see, I had to call register in my main.js to have make tsImport work and it's not clear to me why this is the case.

Please, don't be worried about discussing tsx issues in the ESLint repo. As long as the posts are relevant to the topic (supporting TS config files) It's actually good that everyone involved in the conversation gets notified. Those who don't want to receive notifications can unsubscribe anytime.

This comment has been minimized.

Sign in to view

Copy link
@privatenumber

privatenumber May 11, 2024

Author Owner

Gotcha, thanks for clarifying. This caveat is noted here: https://tsx.is/node/ts-import

Instead of doing a global enhancement via CJS register(), you may want to use tsx.require() so it doesn't affect the run-time. But note, it's basically the same thing as jiti (except it uses esbuild instead of Babel) so it also cannot support TLA.

Going to tackle this problem more extensively soon.

This comment has been minimized.

Sign in to view

Copy link
@fasttime

fasttime May 11, 2024

So, the CJS enhancement is required because tsImport treats eslint.config.ts as a CommonJS file?

The other problem I was mentioning is that tsImport installs a loader with Module.register. The loader may interfere with other tools that use ESLint, and I don't think that Node.js provides a way to remove a loader when it's no longer needed, like after the configs are loaded.

This comment has been minimized.

Sign in to view

Copy link
@privatenumber

privatenumber May 12, 2024 •
edited
Loading

Author Owner

tsx (tsImport) leverages Node.js resolution (doesn't re-implement it), which is TypeScript's Node or Bundler resolution.

Node treats eslint.config.ts as a CommonJS file. This is expected because .ts is the TypeScript counter-part to .js, which can be toggled to be treated as CommonJS or Module depending on package.json#type.

The other problem I was mentioning is that tsImport installs a loader with Module.register.

Yep, there's no official unregister method. But hooks are stackable, and I added a custom unregister method that deactivates the registered hook by immediately going to the next hook:

tsx/src/esm/hook/load.ts

Lines 29 to 31 in efb3509

if (!data.active) {
return nextLoad(url, context);
}

In tsImport, this isn't used because requests are name-spaced. So if the request doesn't contain the namespace as a part of the request, it will be ignored as well:

tsx/src/esm/hook/load.ts

Lines 33 to 35 in efb3509

if (data.namespace && data.namespace !== getNamespace(url)) {
return nextLoad(url, context);
}

I considered opening a feature request for an unregister method in Node, but I figured what I have was sufficient. And considering how long it takes to register a hook in Node (practically negligible but notable if you're micro-benching), I figured there would be an equal amount of overhead to removing it.
test('namespace allows async nested calls', async () => {
await using fixture = await createFixture({
'package.json': JSON.stringify({ type: 'module' }),
Expand Down

0 comments on commit 48f0a75

Please sign in to comment.