Add explicit file extensions imports and make TypeScript checks for them #4217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ESM and Node.JS requires that all relative imports have a file extension. For example, `import x from "abc";` is invalid, as you instead need to do `import x from "abc.js";`. Vite allows it, but it causes issues with other bundlers/software for people that use Mermaid.
Using `moduleResolution: "nodenext"` is the new recommended way of using TypeScript, see https://www.typescriptlang.org/tsconfig#moduleResolution
Update TypeScript requirement to the Version 5.0 release candiate, see https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-rc/
Move the `--emitDeclarationOnly` away from the `tsc` CLI, and into the tsconfig.json file.
TypeScript version 5.0 added supported for importing files with a `.ts` extension, which is what Vite requires in order to import TypeScript files.
Vite doesn't allow importing TypeScript files with `import xx from "xx.js";`. Instead, you must import the files with a `.ts` extension, e.g. `import xx from "xx.ts";`. Because this relies on the new `--allowImportingTsExtensions` option in TypeScript v5.0, most IDEs currently struggle to handle this. Hopefully, this will be fixed once TypeScript v5.0 is officially released!
4 tasks
|
It looks like the This is fine in the generated JavaScript files, because Mermaid uses Vite to transpile TypeScript files. However, the generated Because of that, I'll close this PR. We can consider re-opening it if we ever decide to make a Mermaid v11 release. Or if a future TypeScript release converts these extensions in the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📑 Summary
When using
import ... from "file.js";, many ESM software requires having file-extensions in the imports (for example, Node.JS has this behaviour, see https://nodejs.org/api/esm.html#mandatory-file-extensions).Currently, we don't have any tests for this, which means we've accidentally added this bug a few times before:
dayjsimports #4170eslintlinter config, and fixes some bugs that it caught tbo47/dagre-es#12TypeScript will automatically check this for us if we use the
moduleResolution: "nodenext"setting, see https://www.typescriptlang.org/docs/handbook/esm-node.html📏 Design Decisions
Issues
Only works with TypeScript 5.0 RC
This only works when using TypeScript v5
allowImportingTsExtensions: truesetting. This is because Vite only supports loading TypeScript files usingimport * from "file.ts";(i.e. with.tsextension), but TypeScript v4 only allows loading TypeScript files with the.jsextension.However, TypeScript v5 is not out yet, (only a release candidate is out). Because of that, VS Code and other editors throw TypeScript errors (unless you enable experimental settings).
We may want to avoid merging this PR until TypeScript v5 is officially out (should be out on March 14th 2023).
TypeScript doesn't check
.jsfilesTypeScript currently doesn't check
.jsfiles, so it won't check for the correct import extensions in those files.We could add it by setting
checkJs: truein thetsconfig.jsonfile, but that will probably throw up a lot of other TypeScript errors.mermaid/tsconfig.json
Line 47 in 273a9e7
Alternatively, we could also use the
import/extensionsrule fromeslint-plugin-importonly on JS files.Alternative PR
@remcohaszing has made a similar PR (see #4213) before I did, but has also tried to get it working with Cypress types. Their PR also uses TypeScript v4.9.
Since they made their PR first, I'll rather give priority to merge that PR first, if they get E2E tests working.
📋 Tasks
Make sure you
developbranch