feedback for "fix type definitions"

[romainmenke]

see : #1815

With source :

import type { Declaration, Postcss } from 'postcss';

// return a valid @media rule
export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
	if (typeof dpi === 'boolean') {
		return false;
	}

	// calculate min-device-pixel-ratio and min-resolution
	const dpr = Math.floor(dpi / 96 * 100) / 100;

	const media = postcss.atRule({
		name: 'media',
		params: `(-webkit-min-device-pixel-ratio: ${dpr}), (min-resolution: ${dpi}dpi)`,
		source: decl.source,
	});

	return media;
}

We get these errors :

(!) Plugin typescript: @rollup/plugin-typescript 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.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
                   ~~~~~~~~

(!) Plugin typescript: @rollup/plugin-typescript TS4058: Return type of exported function has or is using name 'RootRaws' from external module "/projects/postcss-preset-env-mono-repo/node_modules/postcss/lib/root" but cannot be named.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
                   ~~~~~~~~

(!) Plugin typescript: @rollup/plugin-typescript TS4058: Return type of exported function has or is using name 'RuleRaws' from external module "/projects/postcss-preset-env-mono-repo/node_modules/postcss/lib/rule" but cannot be named.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
                   ~~~~~~~~

(!) Plugin typescript: @rollup/plugin-typescript TS4058: Return type of exported function has or is using name 'ValueOptions' from external module "/projects/postcss-preset-env-mono-repo/node_modules/postcss/lib/container" but cannot be named.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
                   ~~~~~~~~

(!) Plugin typescript: @rollup/plugin-typescript TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {
                   ~~~~~~~~

[romainmenke]

Pull request with the updated postcss version : https://github.com/csstools/postcss-plugins/pull/945/files

[remcohaszing]

Thanks for the feedback!

That’s some weird error I’ve never seen before. It goes away when adding an explicit return type annotation to that function instead of inferring it.

Of course the goal of #1815 is to be backwards compatible. I found out that it’s fixed, if the types it’s complaining about are exported. Is that solution acceptable?

---

As a side note: The type definitions of postcss-image-set-function are wrong. It’s a commonjs package that specifies module.exports =, but the type definitions specify there’s a default export. Also it’s a dual published package, but the types for the ESM export are missing. As far as I’m aware, there are no build tools available that can correctly create a dual CJS+ESM package from a single code base.

[romainmenke]

As a side note: The type definitions of postcss-image-set-function are wrong. It’s a commonjs package that specifies module.exports =, but the type definitions specify there’s a default export. Also it’s a dual published package, but the types for the ESM export are missing. As far as I’m aware, there are no build tools available that can correctly create a dual CJS+ESM package from a single code base.

Correct, it is the best that can be achieved.
If it needs to be better it is up to TypeScript to facilitate that 🤷

I personally gave up on this and don't want to waste any more of my time on the entire esm vs. commonjs typing mess :D Maybe one day this will be solved by TypeScript/tooling. (pull requests are always welcome :) )

[ai]

@romainmenke how did you fix it (I see CI is green)?

[romainmenke]

CI is green because TypeScript was still able to transpile to JS, even with these errors.
I think this depends on our setup? Which level of warnings/errors causes a non-zero exit code.

[remcohaszing]

I found an example warning in the logs: https://github.com/csstools/postcss-plugins/actions/runs/4686379592/jobs/8304403257?pr=945#step:7:319

[remcohaszing]

I found out that it’s fixed, if the types it’s complaining about are exported. Is that solution acceptable?

@ai If so, I’ll gladly add that. I think that would be the last remaining issue.

[ai]

I think that would be the last remaining issue.

I am too 😅 Maintaining PostCSS could be hard.

I’ll gladly add that

What we could recommend to users who will face it?

[remcohaszing]

Sorry, I think you misunderstood my comment. I meant those errors go away if PostCSS exports the CommentRaws, RootRaws, etc types. This means users will no longer run into this issue.

We could annotate them with an @internal JSDoc comment to indicate they’re not supposed to be used directly if you want.

[ai]

if PostCSS exports the CommentRaws, RootRaws, etc types

Yes, let’s add them for export.

We could annotate them with an @internal JSDoc comment

No, then can be useful for complex types.

[remcohaszing]

Done!

[remcohaszing]

Thanks for the feedback!

[romainmenke]

Still seeing a single warning :

src/index.ts → dist/index.cjs, dist/index.mjs...
(!) Plugin typescript: @rollup/plugin-typescript TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
src/lib/get-media.ts: (13:17)

13 export function getMedia(dpi: number | false, postcss: Postcss, decl: Declaration) {

[ai]

@remcohaszing can we fix it by moving some types from namespaces?

TS7056 happens mostly when you’re putting a lot of schemas and other types into the same namespace.

[remcohaszing]

We really don’t put that many types inside namespaces. I.e. TypeScript 4 and @types/react are examples of projects that use a lot more types inside namespaces.

I managed to resolve it by removing intermediate class declarations for default exports. It even simplifies the types a bit.

I tried this, because I had a hunch it might help. I don’t actually understand why it helps. My guess is the intermediate class declaration counts as another internal type that can’t be resolved, just like CommentRaws etc.

[ai]

@romainmenke can you check the last commit?

[romainmenke]

This seems to resolve all warnings.

The output is a bit different, but this is likely expected?

https://github.com/csstools/postcss-plugins/pull/945/files#diff-f8403694de47f57df79082ec75d1add61900e8e516cf211d02676038b7a7131aR3

[remcohaszing]

I don't know why TypeScript changed the emitted output, but both resolves to the same type, so it's fine. The resolved type annotation is even compatible with the postcss types before #1815

[ai]

We changed types exports, so these changes are expected. But they are not critical. Both options are public API.

[romainmenke]

Awesome!

Thank you for working through this feedback 🎉