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

Surprising module resolution behavior vs. esbuild and bun #442

Open
4 of 6 tasks
andrewbranch opened this issue Dec 18, 2023 · 2 comments
Open
4 of 6 tasks

Surprising module resolution behavior vs. esbuild and bun #442

andrewbranch opened this issue Dec 18, 2023 · 2 comments

Comments

@andrewbranch
Copy link

Acknowledgements

  • I searched existing issues before opening this one to avoid duplicates
  • I understand this is not a place for seek help, but to report a bug
  • I understand that the bug must be proven first with a minimal reproduction
  • I will be polite, respectful, and considerate of people's time and effort

Minimal reproduction URL

https://github.com/andrewbranch/tsx-module-resolution

Version

v4.6.2

Node.js version

v21.1.0

Package manager

npm

Operating system

macOS

Problem & Expected behavior

I uploaded a repo instead of using StackBlitz because it looks like StackBlitz doesn’t support Bun yet, and I wanted to use it as a point of comparison.

This is probably not a bug per se but I wanted to raise this as a design issue that I think should be considered.

Module resolution of node_modules packages does not work how I expected, and differs from what both esbuild and Bun do. It looks like tsx is choosing whether to transform the input to CommonJS based on the file extension / package.json "type", and then performing module resolution from the transformed syntax. So, when you run npm run tsx-ts in the provided repro, the import declaration gets invisibly transformed to a require, and therefore loads the "require" conditional export of pkg. But on the same code, esbuild and Bun both load the "import" conditional export.

Also unlike esbuild and Bun, the behavior changes based on the file extension or package.json "type" of the importing file. When we try the same code from a .mts file (npm run tsx-mts), the explicit require call fails because it’s not processed by tsx; it’s passed on to Node.js in an ESM file.

Expected Actual
npm run tsx-ts { import: 'import.mjs', require: 'require.js', dynamicImport: 'import.mjs' } { import: 'require.js', require: 'require.js', dynamicImport: 'import.mjs' }
npm run tsx-mts { import: 'import.mjs', require: 'require.js', dynamicImport: 'import.mjs' } Errors

All this behavior just falls out of Node.js’s behavior, but it’s not clear to me why tsx can’t or won’t override Node.js here and hide more of the implementation details. I think Bun implemented the intuitive behavior here, and I’ve heard many people suggest tsx as a way to bring Bun-style interop to Node.js, but it seems like the two are fairly far apart on how interop should work.

Moreover, there is no tsconfig.json that can represent what tsx is doing with module resolution. I had previously thought --module esnext --moduleResolution bundler (and soon, --module preserve) were accurate settings, but this difference in conditional "exports" resolution proved me wrong. I’ll have to update the TypeScript docs but it seems there’s no great suggestion I can make for how to type check code written for tsx—--module nodenext is stricter than necessary, but --moduleResolution bundler is inaccurate.

Contributions

  • I plan to open a pull request for this issue
  • I plan to make a financial contribution to this project
@andrewbranch andrewbranch added bug Something isn't working pending triage labels Dec 18, 2023
@privatenumber
Copy link
Owner

Thank you for your detailed issue report, @andrewbranch. Your insight and feedback are highly valued.

Regarding tsx's perceived functionality

Originally, tsx was marketed as an esbuild powered runtime. However, I've shifted away from this description to prevent misunderstandings. Many people filed bugs expecting tsx to mimic esbuild's behavior exactly. In reality, tsx only leverages esbuild for transforming TypeScript (and ESM syntax within CommonJS environments).

I haven't made explicit comparisons to bun, but I acknowledge the need to refine our branding & messaging. Clarifying what users can expect from tsx is a priority I hope to address in early 2024.

Regarding the first issue with npm run tsx-ts

This issue aligns with an existing report: #239

It's not a popular issue and thus not top-priority, but I plan on investigating it.

My plan is to swap the require condition for import in the resolution for ESM-in-CJS files, and transforming further ESM syntax to CommonJS. This approach would be straight forward once Node.js lands complete support for loading CommonJS via Module Hooks.

However, I believe you're suggesting to run ESM syntax in a module context. Initially, tsx operated this way, but it led to compatibility issues with (outdated) packages containing mixed ESM and CJS code. Transforming ESM syntax to CJS was a more practical solution than attempting to fully adapt all CJS functionality in ESM, considering the strict and opt-in nature of modules.

RE: The second issue with npm run tsx-mts

I experimented with compiling via tsc, and running the output yielded the same error. This is not a bug as you noted, but setting "module": "NodeNext" seems to automatically inject createRequire. This was an unexpected feature for me, and I'm not sure how to feel about its implications considering the strictness of modules. But since TypeScript supports this functionality, it seems like a reasonable default to adopt in tsx.

I'm curious—do you know why TypeScript chose to support import a = require('a') in Modules, but not const a = require('a')? Is it for historical, backwards compatibility reasons?

@privatenumber privatenumber removed bug Something isn't working pending triage labels Dec 19, 2023
@andrewbranch
Copy link
Author

Thank for the pointer to #239, I’ll follow that!

However, I believe you're suggesting to run ESM syntax in a module context.

Honestly, I don’t have a strong mental model around what’s really possible with the loader APIs, so I wasn’t implying an implementation suggestion here. Just changing the require condition to import when triggered from import syntax seems good enough to me.

I'm curious—do you know why TypeScript chose to support import a = require('a') in Modules, but not const a = require('a')? Is it for historical, backwards compatibility reasons?

TypeScript files can import and export not just values, but types too. CommonJS constructs use expression-level syntax that wouldn’t have made sense for types:

interface Foo {}
module.exports = Foo; // AssignmentExpression, Foo must be a value
const SomeType = require("a"); // VariableDeclaration, SomeType declares a value

Using alternative syntax made it more clear that (a) these constructs would be transformed before emit, and (b) they can declare meanings besides values. So const a = require('a') never has any special meaning in TypeScript files, even in files that are going to emit CommonJS. Even if we did recognize it, we wouldn’t transform it with the createRequire trick, since we try to transform existing valid JavaScript as little as possible.

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