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

feat(typescript): Update type import specifier rules #381

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Oct 22, 2024

Ref: https://github.com/MetaMask/ocap-kernel/pull/175

This replaces @typescript-eslint/consistent-type-imports with import-x/consistent-type-specifier-style, and an exhortation to enable verbatimModuleSyntax (requires TypeScript >=5) in the tsconfigs of our packages.

Heretofore, we have enforced the existence of type import specifiers, but not that they are stylistically consistent. The options are:

  • "top-level", e.g. import type { a } from 'x'}
  • "inline", e.g. import { type a } from 'x'}

The rule @typescript-eslint/consistent-type-imports has been responsible for this. This rule has an option named fixTypes, but that only applies when a type-only import is not specified as a type-only import. In other words, it enforces the existence of type import specifiers, but not that they're stylistically consistent.

Rather than using @typescript-eslint/consistent-type-imports and its confusingly named options, we are better off enabling verbatimModuleSyntax in our tsconfig files, and enabling import-x/consistent-type-specifier-style. In this way, the existence of type import specifiers is enforced by tsc, and the import-x ESLint plugins ensures that they are consistent. We use the "top-level" style because that results in import type { a } from 'x'; being elided completely from the JS output as opposed to import { type a } from 'x'; which would be emitted as import {} from 'x';. I don't know if that's actually harmful, but it seems surprising, and I don't like surprises.

Supporting documentation:

@rekmarks rekmarks requested review from a team as code owners October 22, 2024 03:53
@rekmarks rekmarks force-pushed the rekm/actually-consistent-type-imports branch from 3436daf to 9205cac Compare October 22, 2024 03:55
@rekmarks
Copy link
Member Author

This will be followed by a bump of the eslint config packages on the module template, which also needs various other updates.

@mcmire
Copy link
Contributor

mcmire commented Oct 22, 2024

I definitely like the idea of being consistent about type imports. I just want to make sure that verbatimModuleSyntax is what we want to go with. There is this section at the end of the documentation you linked:

That does have some implications when it comes to module interop though. Under this flag, ECMAScript imports and exports won’t be rewritten to require calls when your settings or file extension implied a different module system. Instead, you’ll get an error. If you need to emit code that uses require and module.exports, you’ll have to use TypeScript’s module syntax that predates ES2015

Is this something we need to be concerned with?

@mcmire
Copy link
Contributor

mcmire commented Oct 22, 2024

There is also this bit in the ESM/CJS interoperability page in the docs:

As we’ve seen, there is no seamless migration path from transpiled modules to true ESM. But esModuleInterop is one step in the right direction. For those who still prefer to minimize module syntax transformations and the inclusion of the import helper functions, enabling verbatimModuleSyntax is a better choice than disabling esModuleInterop. verbatimModuleSyntax enforces that the import mod = require("mod") and export = ns syntax be used in CommonJS-emitting files, avoiding all the kinds of import ambiguity we’ve discussed, at the cost of ease of migration to true ESM.

and then at the bottom, when talking about caveats around esModuleInterop:

But using verbatimModuleSyntax completely sidesteps the issue with esModuleInterop by forcing CommonJS-emitting files to use CommonJS-style import and export syntax.

So, based on this page, it sounds like verbatimModuleSyntax is largely a net win, but is there anything extra we need to consider here about the effects of this option as it relates to ensuring CJS/ESM compatibility?

@rekmarks
Copy link
Member Author

Good question @mcmire. I suppose the question is whether ts-bridge still works as expected. I'll check with @Mrtenz.

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.

3 participants