-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix type definitions #1815
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 type definitions #1815
Conversation
Many files from postcss are exported. Each of this file is written using the syntax: ```js module.exports = Thing ``` This should be typed as: ```ts export = Thing ``` If such a file needs to export additional types, a namespace should be used. However, it was typed as: ```ts export default thing ``` This was solved at the time in #1459 (by me) by adding the following code to each module: ```js Thing.default = Thing ``` Whilst affective at the time, this was not the correct solution. The introduction of the `"module": "node16"` option in TypeScript made this issue more apparent. I recommend to remove this in a next semver major release. Usage has changed as follows: **cts** ```jsonc // tsconfig.json { "compilerOptions": { "module": "commonjs", "moduleResolution": "node16" } } ``` ```ts // Old import postcss = require('postcss') postcss.default(/* … */) ``` ```ts // New import postcss = require('postcss') postcss(/* … */) ``` **cjs** ```jsonc // tsconfig.json { "compilerOptions": { "checkJs": true, "module": "commonjs", "moduleResolution": "node16" } } ``` ```ts // Old const postcss = require('postcss') postcss.default(/* … */) ``` ```ts // New const postcss = require('postcss') postcss(/* … */) ``` **mts** / **mjs** ```jsonc // tsconfig.json { "compilerOptions": { "module": "node16" } } ``` ```ts // Old import postcss from 'postcss' postcss.default(/* … */) ``` ```ts // New import postcss from 'postcss' postcss(/* … */) ``` **bundler** ```jsonc // tsconfig.json { "compilerOptions": { "allowSyntheticDefaultImports": true, "module": "esnext", "moduleResolution": "node16" } } ``` ```ts // Unchanged import postcss from 'postcss' postcss(/* … */) ``` **ES module interop** (Not recommended for library code!) ```jsonc // tsconfig.json { "compilerOptions": { "esModuleInterop": true, "module": "commonjs", "moduleResolution": "node16" } } ``` ```ts // Unchanged import postcss from 'postcss' postcss(/* … */) ``` When compiling to CJS, it is still possible to write ESM-like syntax, for example: ```jsonc // Same as import postcss = require('postcss') import * as postcss from 'postcss' // Named imports from namespaces are allowed import { root, Root } from 'postcss' ``` The following changes were made to `tsconfig.json`: - `lib` was specified explicitly. By default, TypeScript includes the `dom` types. This is not desirable, it even causes some conflicts with types used by Postcss. - `allowJs` was removed. When this is enabled, TypeScript treats `.d.ts` files matching `.js` files as output. This caused some unexpected issues where TypeScript simply ignored type errors. - `skipLibCheck` was removed. Ideally all types just work together. This can be added again later if necessary. - `esModuleInterop` was removed. Enabling this option causes TypeScript to allow code that is otherwise invalid. This may be useful for CLIs or website, but is dangerous to use for libraries. Correcting these options also highlighted some issues in the tests. These have been resolved. `nanodelay@1` has incorrect type definitions, so it has been marked using `@ts-expect-error` comments. In `test/errors.ts` were some type errors. These were marked as `THROWS`, which signals `check-dts` a type error is expected. This was changed to `@ts-expect-error`, which works too, but it also works with other TypeScript tooling. The `Postcss` type was missing some properties. As a side effect of the corrected types, these are now available. This also affects the `Helper` type. Last, but not least, type definitions have been added for `postcss.mjs`. Closes #1814
FWIW I uploaded the package to https://arethetypeswrong.github.io and it’s all green (except for the missing types). ✅ |
These are redundant if their file path matches the file they represent.
Ok, everything seems to work now. Now my main problem is, it works in CI, but not on my laptop:
Anyway, good enough for me. I’ll perform some cleanups. |
I asked another users with bit TS codebase try this PR. The first feedback #1830
Any idea how to fix it? |
Hi! I'm from the Google Sass team and we use PostCSS via TypeScript internally. From the limited tests I've been able to do (about to go on vacation, so probably won't have time to do anything more thorough any time soon), this looks like it should be a big improvement for us. Our infrastructure has some issues handling Looking forward to seeing this land! |
Thanks for the feedback @jathak! |
Thanks for the huge work. I will release it tomorrow to have a day in case of any problem (now it is a night in my city). |
Released in 8.4.22. |
For a next major release I recommend to remove the default exports, as that has been the source of all problems. I’ll gladly help with that, so feel free to ping me then. |
Many files from postcss are exported. Each of this file is written using the syntax:
This should be typed as:
If such a file needs to export additional types, a namespace should be used.
However, it was typed as:
This was solved at the time in #1459 (by me) by adding the following code to each module:
Whilst affective at the time, this was not the correct solution. The introduction of the
"module": "node16"
option in TypeScript made this issue more apparent. I recommend to remove this in a next semver major release.Usage has changed as follows:
cts
cjs
mts / mjs
bundler
ES module interop (Not recommended for library code!)
When compiling to CJS, it is still possible to write ESM-like syntax, for example:
The following changes were made to
tsconfig.json
:lib
was specified explicitly. By default, TypeScript includes thedom
types. This is not desirable, it even causes some conflicts with types used by Postcss.allowJs
was removed. When this is enabled, TypeScript treats.d.ts
files matching.js
files as output. This caused some unexpected issues where TypeScript simply ignored type errors.skipLibCheck
was removed. Ideally all types just work together. This can be added again later if necessary.esModuleInterop
was removed. Enabling this option causes TypeScript to allow code that is otherwise invalid. This may be useful for CLIs or website, but is dangerous to use for libraries.Correcting these options also highlighted some issues in the tests. These have been resolved.
nanodelay@1
has incorrect type definitions, so it has been marked using@ts-expect-error
comments.In
test/errors.ts
were some type errors. These were marked asTHROWS
, which signalscheck-dts
a type error is expected. This was changed to@ts-expect-error
, which works too, but it also works with other TypeScript tooling.The
Postcss
type was missing some properties. As a side effect of the corrected types, these are now available. This also affects theHelper
type.Last, but not least, type definitions have been added for
postcss.mjs
.Closes #1814