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

fix(types): adds ESM export of types #1457

Merged
merged 1 commit into from
Jul 5, 2022
Merged

fix(types): adds ESM export of types #1457

merged 1 commit into from
Jul 5, 2022

Conversation

lizthegrey
Copy link
Contributor

@netlify
Copy link

netlify bot commented Jul 5, 2022

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit 0e08925
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/62c488115254790009d50c32

@posva posva changed the title fix: adds ESM export of types fix(types): adds ESM export of types Jul 5, 2022
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@posva posva merged commit e82a89c into vuejs:main Jul 5, 2022
@lizthegrey lizthegrey deleted the patch-1 branch July 5, 2022 22:57
@lizthegrey
Copy link
Contributor Author

Confirmed, eve-val/eve-roster#1262 applied cleanly now.

@karlhorky
Copy link

I think this may have introduced a problem with the types for the node16 value for the compilerOptions.moduleResolution option in tsconfig.json - see Are the types wrong:

Imports of "vue-router" under the node16 module resolution setting when the importing module is ESM (its extension is .mts or .mjs, or it has a .ts or .js extension and is in scope of a package.json that contains "type": "module") resolved to CJS types, but ESM implementations.

Screenshot 2023-03-26 at 18 12 00

Copy link
Member

posva commented Apr 24, 2023

What is the problem? The tool isn't clear on whether this is a problem or not. If you have a boiled down reproduction showing a problem, that would help!

@karlhorky
Copy link

Chatting with @andrewbranch (the author of Are The Types Wrong?), it appears that there is more context in the readme on GitHub.

I believe this is the relevant bit:

  • Types are CJS, but implementation is ESM. In the node16 resolution mode, TypeScript detects whether Node itself will assume a file is a CommonJS or ECMAScript module. If the types resolved are detected to be CJS, but the JavaScript resolved is detected to be ESM, this problem is raised. It is often caused by a dependency’s package.json doing something like:
{
  "exports": {
    "types": "./index.d.ts",
    "import": "./index.mjs",
    "require": "./index.js"
  }
}

Because there is no "type": "module" setting here, the .js and .d.ts file will always be interpreted as a CommonJS module. But if the importing file is an ES module, the runtime will resolve to the .mjs file, which is unambiguously ESM. In this case, the module kind of the types misrepresents the runtime-resolved module. This tends to present the most issues for users when default exports are used. In the future, this tool may detect whether an export default might make this problem more severe and give a full explanation of why. In the meantime, you can read the explanation in microsoft/TypeScript#50058 (comment). The simple fix for the example above would be to add an index.d.mts file dedicated to typing the .mjs module, and remove the "types" condition.

Copy link
Member

posva commented Apr 28, 2023

I see, thanks a lot! Since we have no default export, it should be fine. I want to avoid having to add yet another file to the dist folder, it's a maintainer's nightmare 💀

@karlhorky
Copy link

I think it's still indicating there is a problem here to be fixed. Often you can reproduce this with a simple StackBlitz with Node16 module resolution.

@karlhorky
Copy link

I'm not sure how to use vue-router in a standalone way (I'm just using some import I found in some example), but usually this is the type of errors that you would get:

StackBlitz demo: https://stackblitz.com/edit/node-ozajfl?file=tsconfig.json&file=index.ts%3AL6

This expression is not constructable.
  Type 'typeof import("stackblitz:/node_modules/vue-router/dist/vue-router")' has no construct signatures.

Screenshot 2023-04-28 at 14 48 56

@karlhorky
Copy link

karlhorky commented Apr 28, 2023

Ah, looking at this file in the playground, looks like I am doing things wrong.

Maybe this is more along the lines of a normal usage of vue-router@^4 (no errors):

import { createRouter, createWebHistory } from 'vue-router';

const routerHistory = createWebHistory();
const router = createRouter({
  history: routerHistory,
  routes: [{ path: '/', component: null }],
});

Screenshot 2023-04-28 at 14 55 40

StackBlitz demo: https://stackblitz.com/edit/node-d5yw9g?file=tsconfig.json&file=index.ts

@andrewbranch is this a false positive of Are The Types Wrong? in this case?

Or is it:

  • just coincidental that the ESM implementation matches the CJS types
  • but there would be a good chance that the ESM implementation could drift from the CJS types, introducing bugs without any knowledge by maintainers

@andrewbranch
Copy link

Here’s the concrete repro.

// Importing from ESM in Node
import VueRouter from 'vue-router';

const routerHistory = VueRouter.createWebHistory();
const router = VueRouter.createRouter({
  history: routerHistory,
  routes: [{ path: '/', component: null }],
});

Because the types indicate the module is CommonJS, a default import is allowed, and points to the module.exports object of the module. But the reality is that the module is ESM and has no default export, so this program will crash during Node’s module linking phase.

I always advocate for types to correctly represent their module kind, and consider this to be a problem worth fixing. But you can kind of see that you often have to go against conventional usage of the package in order to get yourself in real trouble with this issue.

Copy link
Member

posva commented Apr 28, 2023

Yeah, agree. I don’t think it’s worth complicating the build step and marking the package bigger to make an error for this. It’s already complicated enough 😆

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

Successfully merging this pull request may close these issues.

vue-router 4.1.x missing types export
4 participants