Fix TypeScript exports #6703
Fix TypeScript exports #6703
Conversation
This reverts commit a1a1a8a.
The first change that was made, is that all `declare module`
declarations were replaced. `declare module` is meant for declaring
another module that doesn’t match the package name, for example
`@types/node` uses this to declare `fs`. To accommodate this,
`typeRoots` was replaced with `paths`. Note that this change causes a
lot of indentation changes, but no actual code changes.
Next up was fixing the types of stylelint. Stylelint uses
`module.exports =`. The correct type definition for this is `export =`.
The type definitions also export a number of types. The correct way to
export types in combination with `export =`, is to declare a namespace.
If a namespace has no explicit exports, all members are implicitly
exported. If a namespace has at least one export, all unexported members
are private. I prefer the former. All exported members have been moved
inside the `stylelint` namespace, and had their `export` declarations
removed. All private types are kept outside of the namespace. This
causes more diffs, but no type definitions have been modified.
If `module.exports` is a function or class, static properties are
ideally declared in the namespace. However, `stylelint` extends
`postcss.PluginCreator`, which has a property `postcss`. Because of
this, the type of `stylelint` has been kept as-is.
This change affects all users who rely on synthetic default exports, but
haven’t configured TypeScript correctly. ESM users should specify:
```json
{
"compilerOptions": {
"allowSyntheticDefaultImports": true
}
}
```
or better:
```json
{
"compilerOptions": {
"module": "node16"
}
}
```
CommonJS users who wish to use ESM syntax and transpile, should specify
the following, although I personally recommend against this:
```json
{
"compilerOptions": {
"esModuleInterop": true
}
}
```
The correct way to consume this package using CommonJS is one of the
following. This change is also reflected in the internal stylelint type
annotations and type test:
```ts
// TypeScript
import * as stylelint from 'stylelint'
import stylelint = require('stylelint')
// JavaScript
const stylelint = require('stylelint')
```
|
|
I considered the changeset of the #6695 to be relevant to this PR, so I decided to add that back after reverting. I’m not sure if that matters. 🤔 |
|
@remcohaszing Thanks for the pull request and the follow-up suggestions! This change looks good to me. 👍🏼 @baseballyama Could you confirm this PR can resolve your reported issue #6694? |
`checkJs` implies `allowJs`.
|
I removed I also merged the main branch, and created the following PRs to DefinitelyTyped: |
I believe there is no problem with your decision. 👍🏼 The relevant changeset has been included already: |
|
Just in case, I am trying this PR's changes with my small repo. Here is: I believe this PR works expectedly! |
They have been published to DefinitelyTyped. The published packages are now used as dev dependencies instead.
|
I replaced the vendored types for |
There was a problem hiding this comment.
@remcohaszing A huge thank you for the pull request and the clearly-written explanation!
@ybiquitous Thank you for creating a test repo.
LGTM. I think we can merge and include it in our 15.3.0 release.
|
Sorry for late reply. |
Closes #6694
Refs #6695 (comment)
The first change that was made, is that all
declare moduledeclarations were replaced.declare moduleis meant for declaring another module that doesn’t match the package name, for example@types/nodeuses this to declarefs. To accommodate this,typeRootswas replaced withpaths. Note that this change causes a lot of indentation changes, but no actual code changes.Next up was fixing the types of stylelint. Stylelint uses
module.exports =. The correct type definition for this isexport =. The type definitions also export a number of types. The correct way to export types in combination withexport =, is to declare a namespace.If a namespace has no explicit exports, all members are implicitly exported. If a namespace has at least one export, all unexported members are private. I prefer the former. All exported members have been moved inside the
stylelintnamespace, and had theirexportdeclarations removed. All private types are kept outside of the namespace. This causes more diffs, but no type definitions have been modified.If
module.exportsis a function or class, static properties are ideally declared in the namespace. However,stylelintextendspostcss.PluginCreator, which has a propertypostcss. Because of this, the type ofstylelinthas been kept as-is.This change affects all users who rely on synthetic default exports, but haven’t configured TypeScript correctly. ESM users should specify:
{ "compilerOptions": { "allowSyntheticDefaultImports": true } }or better:
{ "compilerOptions": { "module": "node16" } }CommonJS users who wish to use ESM syntax and transpile, should specify the following, although I personally recommend against this:
{ "compilerOptions": { "esModuleInterop": true } }The correct way to consume this package using CommonJS is one of the following. This change is also reflected in the internal stylelint type annotations and type test:
I have some follow-up suggestions that are out of scope of this PR.
@types/jest, fixing their types, and removing theexcludeoption fromtsconfig.json.checkJsimpliedallowJs. I suggest to remove it.typesfolder to DefinitelyTyped. This will help others, and keep the stylelint codebase focused on stylelint.