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 type definitions #1815

Merged
merged 17 commits into from Apr 15, 2023
Merged

Fix type definitions #1815

merged 17 commits into from Apr 15, 2023

Conversation

remcohaszing
Copy link
Contributor

Many files from postcss are exported. Each of this file is written using the syntax:

module.exports = Thing

This should be typed as:

export = Thing

If such a file needs to export additional types, a namespace should be used.

However, it was typed as:

export default thing

This was solved at the time in #1459 (by me) by adding the following code to each module:

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

// tsconfig.json
{
  "compilerOptions": {
    "module": "commonjs",
    "moduleResolution": "node16"
  }
}
// Old
import postcss = require('postcss')

postcss.default(/* … */)
// New
import postcss = require('postcss')

postcss(/* … */)

cjs

// tsconfig.json
{
  "compilerOptions": {
    "checkJs": true,
    "module": "commonjs",
    "moduleResolution": "node16"
  }
}
// Old
const postcss = require('postcss')

postcss.default(/* … */)
// New
const postcss = require('postcss')

postcss(/* … */)

mts / mjs

// tsconfig.json
{
  "compilerOptions": {
    "module": "node16"
  }
}
// Old
import postcss from 'postcss'

postcss.default(/* … */)
// New
import postcss from 'postcss'

postcss(/* … */)

bundler

// tsconfig.json
{
  "compilerOptions": {
    "allowSyntheticDefaultImports": true,
    "module": "esnext",
    "moduleResolution": "node16"
  }
}
// Unchanged
import postcss from 'postcss'

postcss(/* … */)

ES module interop (Not recommended for library code!)

// tsconfig.json
{
  "compilerOptions": {
    "esModuleInterop": true,
    "module": "commonjs",
    "moduleResolution": "node16"
  }
}
// Unchanged
import postcss from 'postcss'

postcss(/* … */)

When compiling to CJS, it is still possible to write ESM-like syntax, for example:

// 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

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
@remcohaszing
Copy link
Contributor Author

FWIW I uploaded the package to https://arethetypeswrong.github.io and it’s all green (except for the missing types). ✅

test/errors.ts Outdated Show resolved Hide resolved
lib/container.d.ts Outdated Show resolved Hide resolved
These are redundant if their file path matches the file they represent.
package.json Outdated Show resolved Hide resolved
@remcohaszing
Copy link
Contributor Author

remcohaszing commented Feb 16, 2023

CI is failing due to a bug in check-dts. I already spotted the cause, but currently don’t have the time to fix it. I’ll get to it somewhere in the upcoming weekend / week I think.

I decided to not go and fix check-dts. There are already more established tools out there, such as:

@remcohaszing
Copy link
Contributor Author

Ok, everything seems to work now. Now my main problem is, it works in CI, but not on my laptop:

✖ Check types

✖ node_modules/.pnpm/@types+node@18.11.18/node_modules/@types/node/globals.d.ts:67:13: Type error TS2502
  'AbortController' is referenced directly or indirectly in its own type annotation.

✖ node_modules/.pnpm/@types+node@18.11.18/node_modules/@types/node/globals.d.ts:74:13: Type error TS2502
  'AbortSignal' is referenced directly or indirectly in its own type annotation.

Anyway, good enough for me. I’ll perform some cleanups.

@ai
Copy link
Member

ai commented Apr 13, 2023

I asked another users with bit TS codebase try this PR. The first feedback #1830

TS4058: Return type of exported function has or is using name 'CommentRaws' from external module "/projects/postcss-preset-env-mono-repo/node_modules/postcss/lib/comment" but cannot be named.

Any idea how to fix it?

@jathak
Copy link

jathak commented Apr 13, 2023

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 export default, so since PostCSS 8 we've had to manually patch the types to work around that, but with this PR it looks like they should work for us without any changes.

Looking forward to seeing this land!

@remcohaszing
Copy link
Contributor Author

Thanks for the feedback @jathak!

@ai ai merged commit a9a157a into postcss:main Apr 15, 2023
7 checks passed
@ai
Copy link
Member

ai commented Apr 15, 2023

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).

@ai
Copy link
Member

ai commented Apr 16, 2023

Released in 8.4.22.

@remcohaszing
Copy link
Contributor Author

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.

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.

"module": "node16" error: This expression is not callable
3 participants