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

Problems with package.json wildcard exports #3201

Closed
yzrmn opened this issue Jun 29, 2023 · 13 comments
Closed

Problems with package.json wildcard exports #3201

yzrmn opened this issue Jun 29, 2023 · 13 comments

Comments

@yzrmn
Copy link

yzrmn commented Jun 29, 2023

Hi!

redgeometry is a hybrid ESM/CJS NPM package that also ships its source files. I realized that if the consuming application uses "moduleResolution": "Node16" the package also needs to specify the export in package.json. However, it looks like esbuild does not handle it correctly.

Example project setup

./node_modules/redgeometry/package.json (exports altered)

    ...
    "exports": {
        ".":{
            "import": "./dist/index.js",
            "require": "./dist/index.cjs"
        },
        "./src/*": {
            "default": "./src/*"
        }
    },
    ...

./src/index.ts

import { Vector2 } from "redgeometry/src/primitives/vector.js";

const v = new Vector2(1, 2);
console.log(v.length);

./package.json

{
    "name": "esbuild-exports",
    "version": "0.1.0",
    "type": "module",
    "dependencies": {
        "redgeometry": "0.3.0"
    },
    "devDependencies": {
        "esbuild": "^0.18.10",
        "typescript": "^5.1.4"
    },
    "scripts": {
        "init": "npm install",
        "check": "tsc",
        "serve": "esbuild ./src/index.ts --serve --bundle --outdir=./dist"
    }
}

./tsconfig.json

{
    "compilerOptions": {
        "esModuleInterop": true,
        "isolatedModules": true,
        "module": "ESNext",
        "moduleResolution": "Node16",
        "noEmit": true,
        "strict": true,
        "target": "ESNext",
        "verbatimModuleSyntax": true
    }
}

TSC behaves as expected with the export, but esbuild complains:

X [ERROR] Could not resolve "redgeometry/src/primitives/vector.js"

    src/index.ts:1:24:
      1 │ import { Vector2 } from "redgeometry/src/primitives/vector.js";
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  The module "./src/primitives/vector.js" was not found on the file system:

    node_modules/redgeometry/package.json:23:23:
      23 │             "default": "./src/*"
         ╵                        ~~~~~~~~~

If we change:

  • "type": "commonjs"
  • "moduleResolution": "Node"
  • import { Vector2 } from "redgeometry/src/primitives/vector" (extensionless)

Again, TSC is fine with that, but esbuild gives:

X [ERROR] Could not resolve "redgeometry/src/primitives/vector"

    src/index.ts:1:24:
      1 │ import { Vector2 } from "redgeometry/src/primitives/vector";
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  The module "./src/primitives/vector" was not found on the file system:

    node_modules/redgeometry/package.json:23:23:
      23 │             "default": "./src/*"
         ╵                        ~~~~~~~~~

  Import from "redgeometry/src/primitives/vector.ts" to get the file
  "node_modules/redgeometry/src/primitives/vector.ts":

    src/index.ts:1:58:
      1 │ import { Vector2 } from "redgeometry/src/primitives/vector";
        │                                                           ^
        ╵                                                           .ts

Interestingly, changing the export to

        "./src/*": {
            "default": "./src/*.ts"
        }

satisfies esbuild in this case.

But this obviously would not work for the original setup ("moduleResolution": "Node16")...

TSC:

src/index.ts:1:25 - error TS2307: Cannot find module 'redgeometry/src/primitives/vector.js' or its corresponding type declarations.

1 import { Vector2 } from "redgeometry/src/primitives/vector.js";
                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

esbuild:

X [ERROR] Could not resolve "redgeometry/src/primitives/vector.js"

    src/index.ts:1:24:
      1 │ import { Vector2 } from "redgeometry/src/primitives/vector.js";
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  The module "./src/primitives/vector.js.ts" was not found on the file system:

    node_modules/redgeometry/package.json:23:23:
      23 │             "default": "./src/*.ts"
         ╵                        ~~~~~~~~~~~~

It looks to me that esbuild does not apply the same rules when resolving those import paths. Is it reasoanble to expect that esbuild behaves similar to TSC in this case? Or is there a different solution that I am missing?

@evanw
Copy link
Owner

evanw commented Jun 29, 2023

Or is there a different solution that I am missing?

Does allowImportingTsExtensions allow you to import with .ts here?

redgeometry is a hybrid ESM/CJS NPM package that also ships its source files.

Keep in mind that this is a use case that TypeScript is not designed for, and which is known to not work well. There's a long thread with commentary from the TypeScript team which talks about this a bit if you're interested. Basically npm packages authored in TS are expected to publish generated JS code, not TS source code, because the behavior of that TS code depends on the TS compilation settings used at the time of authoring. To avoid behavior changes, these settings should be in control of the package author, not the package consumer. So the best solution here would probably be for this package to publish the generated JS files like it's supposed to be doing.

Edit: Playground link for this issue

@yzrmn
Copy link
Author

yzrmn commented Jun 29, 2023

Does allowImportingTsExtensions allow you to import with .ts here?

Yes, this would let me import "redgeometry/src/primitives/vector.ts" with esbuild and TSC (Node). To my surprise Node16/ NodeNext and Bundler work without that setting.

Keep in mind that this is a use case that TypeScript is not designed for, and which is known to not work well. There's a #3019 with commentary from the TypeScript team which talks about this a bit if you're interested. Basically npm packages authored in TS are expected to publish generated JS code, not TS source code, because the behavior of that TS code depends on the TS compilation settings used at the time of authoring. To avoid behavior changes, these settings should be in control of the package author, not the package consumer. So the best solution here would probably be for this package to publish the generated JS files like it's supposed to be doing.

ESM and CJS (redgeometry import) are bundled JS files within this package. The published source files also work with the source maps (sourcesContent: false) and are optional imports (you need to explicitly import redgeometry/src/*). To quote you again:

I suspect there are some people that are doing this intentionally (via Hyrum's Law). For example, I could see it being useful to ensure that const enum is inlined, since bundling from the original source code results in better code in that case. It would certainly be simpler for esbuild to refuse to process .ts files in node_modules, but I suspect doing that will break some people. I swear I remember hearing about some people doing this but I can't find it right now.

This is what's happening here. I would like to give more control (or have it myself) for applications that consume the package. Inlining, dead code elimination and conditional code (e.g. asserts) come to mind. Also debugging complex cases inside the app is so much easier/productive.
The tradeoff is to adhere to a common denominator in terms of tsconfig and typescript version, which should be pretty reasonable in this case.

So is the expected behavior of esbuild to NOT resolve a node module import from package/subdir/file.js to package/subdir/file.ts like it would do for regular/relative imports?

@evanw
Copy link
Owner

evanw commented Jun 29, 2023

No I think it should be ok for esbuild to do this in this case, since there are no .js files. I can try to get esbuild to do that here. But if there are .js files present, I think esbuild should still prefer the .js files to the .ts files in this case.

@yzrmn
Copy link
Author

yzrmn commented Jun 29, 2023

I agree that the actual file in the import should always be preferred.

I did some more tests and found that TSC seemingly ignores the exports with "moduleResolution": "Node" while esbuild always respects them because it simply does not read the field (https://esbuild.github.io/content-types/#tsconfig-json). Without any exports both TSC and esbuild work as expected (as far as I can tell with any of the module resolutions). Your proposed change would probably resolve the major use cases for files with extensions (if not all) when using the exports field.

But I'm not sure about how extensionless imports with Node resolution should be resolved with exports (second example). Would it be more consistent to do the same as TSC, i.e. to ignore the export field in this case?

@evanw
Copy link
Owner

evanw commented Jun 29, 2023

I'm not sure exactly what you're saying by "ignores the exports" because you haven't provided a self-contained way to reproduce your issue. But I suspect that extensionless imports should not work if exports is present. I believe the exports field was deliberately designed to make extensionless imports not work (they are viewed as a mistake with node's original design). So extensionless imports wouldn't work with node's native ESM support, and shouldn't work in esbuild either. But I can look more into this if/when you provide a self-contained way to reproduce your issue.

@yzrmn
Copy link
Author

yzrmn commented Jun 29, 2023

Sorry for not being clearer :) I was just pointing out a case when esbuild respects package.json exports and TSC does not. In the table Comparison with existing module resolution settings it is noted that the module resolutions Classic and Node do not support package.json exports.

So in this example it works with TSC but not with esbuild because TSC ignores the package.json export while esbuild, not knowing about the module resolution, respects it (if you delete the exports field from node_modules/some-package/package.json it works).

I'm actually fine with NOT supporting extensionless import paths for package.json exports but wanted to draw attention to this (slightly related) matter. My motivation was to reduce cases where TSC and esbuild differ in their behavior.

@evanw
Copy link
Owner

evanw commented Jun 29, 2023

I think perhaps esbuild shouldn't respect moduleResolution even though it could. It seems like moduleResolution is a tool that helps you adapt TypeScript to the module system that is being used to run your code, not a tool that helps the module system that is being used to run your code adapt to TypeScript. For example, I believe that the intent of adding the bundler module resolution mode is so that you can set "moduleResolution": "bundler" when using a bundler such as esbuild.

@yzrmn
Copy link
Author

yzrmn commented Jun 29, 2023

Fair point, I didn't think about it that way and I agree with using the Bundler module resolution. That's what it's there for :)

@jedwards1211
Copy link

jedwards1211 commented Jul 11, 2023

I believe the exports field was deliberately designed to make extensionless imports not work

Extensionless imports from packages are still possible. For example import pick from 'lodash-es/pick' doesn't work because lodash-es doesn't declare an export map; you have to import from 'lodash-es/pick.js'. But you'd be able to omit the extension if it declared the following export map:

  "exports": {
    "./*": "./*.js"
  }

Them importing from 'lodash-es/pick' would resolve via the export map to node_modules/lodash-es/pick.js.

@jedwards1211
Copy link

Inlining, dead code elimination and conditional code (e.g. asserts)

Aren't all of these possible on JS files, at least if they're ESM?

@evanw evanw closed this as completed in cc25614 Jul 12, 2023
@yzrmn
Copy link
Author

yzrmn commented Jul 14, 2023

@evanw: The fix works great, thanks! I have tested multiple configurations and esbuild lines up with TSC now :)

Aren't all of these possible on JS files, at least if they're ESM?

Probably yes, but then the library would need to distribute the bundled files with defines (e.g. debug or trace) that would have been optimized away otherwise. In that case users of the library must take care of the optimizations themselves either way (if they don't want the performance penalty).

Extensionless imports from packages are still possible.

Yes! I have found that out as well. In the example from the OP you could import (...)/vector, (...)/vector.js and (...)/vector.ts with:

    "./src/*": "./src/*.ts",
    "./src/*.ts": "./src/*.ts",
    "./src/*.js": "./src/*.ts"

With esbuild 0.18.12 the following would enable both imports (...)/vector.js and (...)/vector.ts:

    "./src/*": "./src/*"

@jedwards1211
Copy link

Probably yes, but then the library would need to distribute the bundled files with defines (e.g. debug or trace) that would have been optimized away otherwise

I'm curious what you mean, do you have an example?

@yzrmn
Copy link
Author

yzrmn commented Jul 15, 2023

Sure, let's consider this example which is set up for release. The validation function is completly removed because esbuild recognizes that the if-condition is always false. For debug builds we would use define: { "ENV_DEBUG": "true" } so that the validation function is always called. In both cases ENV_DEBUG vanishes from the resulting code.

Now we want users of this library to decide whether they want the runtime validation to happen or not. Then ENV_DEBUG needs to stay in the code (like this) and be defined by the consuming application or its build process. But not all build tools possess the same level of optimizations as esbuild, so the application might end up checking ENV_DEBUG at runtime (which could affect performance in hot loops, especially if your code is drenched in checks).

Arguably, we could ship additional debug bundles and I have seen some libraries doing it. But maybe we want multiple ENV_TRACE_FEATURE* defines as well (more help to track down bugs in the context of the application). Distributing all those permutations would be silly of course.

So I think it's reasonable to publish libraries with full release optimizations and include the original source files for advanced debugging scenarios or fine grained access.

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