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 (or incorrect) priorities with package.json exports fields? #46334

Closed
DanielRosenwasser opened this issue Oct 13, 2021 · 26 comments
Closed
Labels
Discussion Issues which may not have code impact

Comments

@DanielRosenwasser
Copy link
Member

I'm trying a scenario of module: nodenext with Vue.js. I hit a few issues with resolution of declaration files.

Here's the current Vue.js declarations package.json:

{
    "name": "vue",
    "version": "3.2.20",
    "description": "The progressive JavaScript framework for buiding modern web UI.",
    "main": "index.js",
    "module": "dist/vue.runtime.esm-bundler.js",
    "types": "dist/vue.d.ts",
    // ...
    "exports": {
      ".": {
        "import": {
          "node": "./index.mjs",
          "default": "./dist/vue.runtime.esm-bundler.js"
        },
        "require": "./index.js"
      },
      // ...
      "./package.json": "./package.json"
    }
    // ...
}

However, referencing this in a project results in the following error:

import * as Vue from "vue";
//                   ~~~~~
// error
// Could not find a declaration file for module 'vue'. 'USER_DIR/hackathon/vue-proj/node_modules/vue/index.js' implicitly has an 'any' type.
//  Try `npm i --save-dev @types/vue` if it exists or add a new declaration (.d.ts) file containing `declare module 'vue';`

This kind of makes sense - I think you could argue that this isn't configured right for moduleResolution: node12 or later.

I was able to get this working by adding

 "exports": {
     ".": {
     "import": {
         "node": "./index.mjs",
         "default": "./dist/vue.runtime.esm-bundler.js"
     },
     "require": "./index.js",
+    "types": "./dist/vue.d.ts"
     },
 }

But the following DID NOT work.

 "exports": {
     ".": {
     "import": {
         "node": "./index.mjs",
         "default": "./dist/vue.runtime.esm-bundler.js"
+        "types": "./dist/vue.d.ts"
     },
     "require": "./index.js",
     },
 }

That part seems like a bug, right?

@DanielRosenwasser DanielRosenwasser changed the title Surprising (or incorrect) priorities with export maps? Surprising (or incorrect) priorities with package.json export fields? Oct 13, 2021
@andrewbranch
Copy link
Member

This kind of makes sense - I think you could argue that this isn't configured right for moduleResolution: node12 or later.

My assumption was that a top-level types should merge with the . of an export map in order to continue offering typing support for the top-level import of all existing libraries that are already using export maps. I logged this at #46281.

@andrewbranch andrewbranch added the Bug A bug in TypeScript label Oct 13, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.1 milestone Oct 13, 2021
@weswigham
Copy link
Member

That part seems like a bug, right?

Nope. Is the referencing file cjs mode or is esm mode? An import condition is only going to be applicable for an esm mode import - so a cjs mode import (eg, an import in a js file in a package without type:module) isn't going to use the import condition for its imports - it uses the require condition instead.

@weswigham
Copy link
Member

My assumption was that a top-level types should merge with the . of an export map in order to continue offering typing support for the top-level import of all existing libraries that are already using export maps. I logged this at #46281.

types is a TS-specific main, which exports blocks - I don't see why it shouldn't also block types.

@andrewbranch
Copy link
Member

Is the referencing file cjs mode or is esm mode?

I missed that the package.json didn’t have "type": "module". @DanielRosenwasser what was the file extension of the importing file?

@DanielRosenwasser
Copy link
Member Author

I was importing from a module (.ts file with "type": "module").

@DanielRosenwasser
Copy link
Member Author

types is a TS-specific main, which exports blocks - I don't see why it shouldn't also block types.

I think this is fair, but it highlights 3 things to me

  1. We should really give a more accurate error message

    "A 'types' field was found in this package's 'package.json', but was not used because an 'exports' field was found and took priority over the top-level 'main' and 'types' field."

    Probably needs to be word-smithed, but I think it would be helpful.

  2. We need to be cautious in our messaging - over-eager people will try to use this in regular projects, and existing packages aren't ready to accommodate them.

  3. We probably should help package authors get ready for node12+ resolution modes.

@DanielRosenwasser
Copy link
Member Author

In any case, it's a bug that this one didn't work, right?

 "exports": {
     ".": {
     "import": {
         "node": "./index.mjs",
         "default": "./dist/vue.runtime.esm-bundler.js"
+        "types": "./dist/vue.d.ts"
     },
     "require": "./index.js",
     },
 }

@weswigham
Copy link
Member

weswigham commented Oct 14, 2021

default is going to be matched before types, because the default condition is always set, and the object is ordered. (So you probably wanna list the type condition first, since it's not usually set except by TS)

@DanielRosenwasser DanielRosenwasser changed the title Surprising (or incorrect) priorities with package.json export fields? Surprising (or incorrect) priorities with package.json exports fields? Oct 15, 2021
@DanielRosenwasser
Copy link
Member Author

Oof that's really confusing. Shouldn't import take priority over types too though?

@weswigham
Copy link
Member

The mistake is thinking they're prioritized - they're not. They're either on or off, and the first condition (in object insertion order) that is on is selected.

@Jamesernator
Copy link

Jamesernator commented Oct 26, 2021

default is going to be matched before types, because the default condition is always set, and the object is ordered. (So you probably wanna list the type condition first, since it's not usually set except by TS)

I don't think this is required for TypeScript to do, Node's official guidance is:

When using environment branches, always include a "default" condition where possible. Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation, which helps avoid these JS environments from having to pretend to be existing environments in order to support packages with conditional exports. For this reason, using "node" and "default" condition branches is usually preferable to using "node" and "browser" condition branches.

However the point about

Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation,

Doesn't really apply to TypeScript when looking for types as the "universal implementation" is not neccessarily what is desired.

I think the easiest way for TypeScript to behave in an expected way would be to do two passes on conditions, first do a pass for ["import", "types"], if this exists then stop, otherwise do a pass for ["import", "default"], if this exists then it should point to a module file of some kind, in which case lookup the associated .d.ts (or .d.mts, etc).

i.e. The logic would be like:

const typeFile = lookupExportMap(["import", "types"]);
if (exists(typeFile)) {
    // use it
    return (...);
}

// If an explicit type mapping doesn't exist look for a module file
const moduleFile = lookupExportMap(["import", "default"]);

if (!exists(moduleFile)) {
    throw new Error("No such module");
}

if (moduleFile.endsWith(".js")) {
    const dTs = replaceExtension(moduleFile, ".d.ts");
    if (exists(dTs)) {
        // Use the d.ts file
        return (...);
    }
    // Else try parsing moduleFile for JSDOC
    return (...);
}
// And similar for .cjs/.mjs
// and whatever other handling for .json etc

@weswigham
Copy link
Member

Export maps, very explicitly and by their design, follow the priority order listed in the conditional export object. That... Can be confusing at times; but not respecting that in TS would only lead to more confusion.

@Jamesernator
Copy link

Jamesernator commented Oct 26, 2021

Export maps, very explicitly and by their design, follow the priority order listed in the conditional export object.

The above suggestion does not change this, it just queries twice with two sets of conditions, for each of the queries it still does things in order as per usual.

That... Can be confusing at times; but not respecting that in TS would only lead to more confusion.

I mean either case is confusing, however I think allowing it to work is still perfectly explainable, TypeScript tries import,types, then tries import,default. Once people know that the resolution logic is pretty basic, and is basically how "typesVersions" works today (i.e. "typesVersions" is checked FIRST, then paths are looked up if that fails).

but not respecting that in TS would only lead to more confusion.

There is also the fact the fact "exports" is so generic is so that tools like TypeScript can declare such data, and people probably don't expect "types" to not win in this case. The reason import,node,default and such work as a single condition is because the end target only expects a single type of resource, a module, what kind of module is arbitrary. However TypeScript doesn't understand arbitrary modules, it specifically needs types, doing a query for types first and falling back to an aribitrary module (and replacing its extension with .d.ts or whatever) actually makes sense, as TS is meant to deal with types.

@weswigham
Copy link
Member

weswigham commented Oct 26, 2021

TypeScript doesn't understand arbitrary modules, it specifically needs types, doing a query for types first and falling back to an aribitrary module (and replacing its extension with .d.ts or whatever) actually makes sense, as TS is meant to deal with types.

Except we kinda do. First, we'll look for .d.ts files next to the js (or extensionless!) file we find, so in that way, every js resolution is a valid potential resolution, second we do load js files directly when allow js is set, so we need to follow js resolution rules, and lastly, every condition needs to be set at the same time to support compound (ie, nested) conditions.

@bmeck
Copy link

bmeck commented Nov 4, 2021

Likely there should be a warning if anything comes after a "default" condition.

@andrewbranch andrewbranch added the Discussion Issues which may not have code impact label Nov 4, 2021
@andrewbranch andrewbranch removed the Bug A bug in TypeScript label Nov 4, 2021
@andrewbranch andrewbranch removed this from the TypeScript 4.5.1 milestone Nov 4, 2021
@Jamesernator
Copy link

and lastly, every condition needs to be set at the same time to support compound (ie, nested) conditions.

Right I see what you mean.

Likely there should be a warning if anything comes after a "default" condition.

Yeah this would solve the problem, is there an intention to add this into Node or should this be part of say text editors, or even other tooling?

@andrewbranch
Copy link
Member

We’re looking into adding it to tsc and surfacing it in VS Code.

@thetutlage
Copy link

Should this impact the apps not using node12 and no explicit type is defined in package.json file?

Coz, I have an application that breaks after upgrading to 4.5. Here is a sample repo to reproduce the issue. https://github.com/thetutlage/Typescript-4-5-regression

Happy to provide more info if required :)

thetutlage added a commit to adonisjs/core that referenced this issue Nov 21, 2021
Our lovely TypeScript is finding every possible way to break things on every release.
I cannot find any documentation on "export maps" behavior, except these issues

microsoft/TypeScript#46860
microsoft/TypeScript#46334
@andrewbranch
Copy link
Member

@thetutlage that’s #46770, which I believe is a bug—didn’t realize it was affecting non-node{12,next} resolution modes. Thanks for the repro!

@andrewbranch
Copy link
Member

Actually #46770 has a couple different things going on so it’s hard to say what’s what. The issue you reproduced is caused by this:

const moduleResolutionState: ModuleResolutionState = { compilerOptions: options, host, traceEnabled, failedLookupLocations, packageJsonInfoCache: cache, features: NodeResolutionFeatures.AllFeatures, conditions: ["node", "require", "types"] };

Type reference directives are always being resolved with NodeResolutionFeatures.AllFeatures, which prevents us from looking at types and main here:

if (!(state.features & NodeResolutionFeatures.Exports)) {

@andrewbranch
Copy link
Member

^ fixed by #47007

@otakustay
Copy link

Also I see nodenext mode do not have a default types resolution, when package.json contains neither types nor exports, but a index.d.ts is placed in package root, no types is resolved, is this intended?

@andrewbranch
Copy link
Member

Yes, node12/nodenext has no special handling of index files.

makeeno added a commit to makeeno/fast-check that referenced this issue Mar 17, 2022
When trying to use `fast-check` with TypeScript where  `moduleResolution` is set to `Node12` or `NodeNext` we get the following error:

> Could not find a declaration file for module 'fast-check'. '/home/makeen/q2web/Dev/openapi-codegen/node_modules/fast-check/lib/esm/fast-check.js' implicitly has an 'any' type.

> Try `npm i --save-dev @types/fast-check` if it exists or add a new declaration (.d.ts) file containing `declare module 'fast-check';`ts(7016)

This change fixes the issue.

I think this might be related: microsoft/TypeScript#46334
dubzzz pushed a commit to dubzzz/fast-check that referenced this issue Mar 17, 2022
When trying to use `fast-check` with TypeScript where  `moduleResolution` is set to `Node12` or `NodeNext` we get the following error:

> Could not find a declaration file for module 'fast-check'. '/home/makeen/q2web/Dev/openapi-codegen/node_modules/fast-check/lib/esm/fast-check.js' implicitly has an 'any' type.

> Try `npm i --save-dev @types/fast-check` if it exists or add a new declaration (.d.ts) file containing `declare module 'fast-check';`ts(7016)

This change fixes the issue.

I think this might be related: microsoft/TypeScript#46334
@thw0rted
Copy link

thw0rted commented Aug 2, 2022

It sounds like the end result of the conversation abve is that if a package has a top level types, but also has a matching exports value with no types, TS will not load any typings (though it could in theory check the matched JS specifier directly when allowJS is set). Is that correct?

If so, what advice will you give to those who consume packages like this? Declaring types but not exports.{something}.types is basically always an error, right? Is there some mechanism for me to depend on package foo but tell TS to either ignore its exports field, or override it in some way? If not, does that just mean I have to get the library authors to fix their package or make a fork of it myself?

Also: I don't know if it was brought up previously, but this behavior has a sort of impedance-mismatch with existing frontend bundlers -- webpack respects the exports field today, but bunding TS for browser consumption through webpack means types are pulled from top level types, not the matching exports mapping. Is this issue the right place to address that?

@andrewbranch
Copy link
Member

Declaring types but not exports.{something}.types is basically always an error, right?

First of all, note that if main, or any part of exports points to a JavaScript file, and there is no corresponding types key when resolving through that package.json path, the compiler will look for a declaration file next to that JavaScript file and use that. So a foolproof way to write a valid project structure and package.json file is just to publish your declaration files in the same place as their partner JS files and literally never write types anywhere in your package.json. The only time an author has to start writing types keys is if they break that structure, e.g. by putting JS in one directory and types in another. (This seems to be one of package authors’ absolute favorite ways to make their own lives harder. Maybe it’s the default for some third party tool like rollup?)

To answer your other questions:

In node16/nodenext, it is expected for the top-level types to be ignored in the presence of exports, because Node ignores the top-level main in the presence of exports. (The point of a targeted moduleResolution is to maintain a parallel logic like this as perfectly as we can, so there is parity between what works at compile time and what works at runtime.)

Is there some mechanism for me to depend on package foo but tell TS to either ignore its exports field, or override it in some way?

Nothing specifically for this problem, but tsconfig paths will probably let you hack something together?

does that just mean I have to get the library authors to fix their package

Please 🙏

but bunding TS for browser consumption through webpack

My take is that it’s basically a coincidence that TypeScript worked reasonably well with bundlers for years without complaints. Part of the promise of bundlers was that you can develop your frontend projects like Node, using dependencies from npm, and so bundlers copied Node’s module resolution strategy, so --moduleResolution node was appropriate for TS, and then enhanced it in ways that TypeScript could sometimes model with paths or pattern ambient modules and otherwise felt out of scope. But now, Node and bundlers have both diverged away from --moduleResolution node in meaningful ways and different directions. We shipped support for the direction Node went, but we effectively now have no support for what bundlers are doing. This is 90% of what I have been thinking about and working on for the last month. Hoping to publish a proposal this week. So no, this is not the right issue to address that, but there isn’t really a canonical issue for it. Stay tuned.

Also, I think this issue can be closed?

@thw0rted
Copy link

thw0rted commented Aug 3, 2022

Excellent answer as always, thanks Andrew. One or two follow-ups though:

a foolproof way to write a valid project structure and package.json file is just to publish your declaration files in the same place as their partner JS files and literally never write types anywhere in your package.json

This sounds like a sentence that should appear somewhere on a "for library authors" page in the TS handbook. Does it?

tsconfig paths will probably let you hack something together? / {"please" get authors to fix their package}

This isn't a hypothetical. When Webpack started respecting exports, import { ... } from "somelib/assets/file.css" started to throw an "is not exported from package" error. I have an open issue with a pretty popular library that is coming up on its second anniversary (!) while they deliberate how to fix it. At least for the asset, I can work around it by transforming the module specifier into an absolute path using Webpack's resolve.alias. I think you're going to see a lot of new bug reports here in the next couple of months as library authors struggle to migrate to exports and mess up types resolution in the process, if there's not an easy path for consumers to work around the issue.

mizdra added a commit to mizdra/happy-css-modules that referenced this issue Sep 18, 2022
bahlo added a commit to axiomhq/axiom-js that referenced this issue Jun 26, 2023
Otherwise projects might complain, here's Vue:

> src/App.vue:5:23 - error TS7016: Could not find a declaration file for module '@axiomhq/js'. '/Users/arne/Developer/axiomhq/axiom_ts_issue/node_modules/@axiomhq/js/dist/esm/index.js' implicitly has an 'any' type.
>   There are types at '/Users/arne/Developer/axiomhq/axiom_ts_issue/node_modules/@axiomhq/js/dist/types/index.d.ts', but this result could not be resolved when respecting package.json "exports". The '@axiomhq/js' library may need to update its package.json or typings.

See also microsoft/TypeScript#46334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

8 participants