Skip to content

[css-tokenizer] add types entry in package.json for older projects #1437

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

Closed
wants to merge 1 commit into from

Conversation

ssttevee
Copy link

namely where compilerOptions.moduleResolution < "node16" in tsconfig.json

namely where `compilerOptions.moduleResolution < "node16"` in tsconfig.json
@ssttevee
Copy link
Author

ssttevee commented Jul 19, 2024

This is not about commonjs, it's more about compatibility.

Merely including this field does not support commonjs, but gives bundlers and typescript more options to find the type definitions if there are other configuration constraints that prevent reading from the exports.

Edit: I apologize if this has already been thoroughly discussed elsewhere, but it seems to me that it is intentionally breaking compatibility unnecessarily.

@ssttevee ssttevee closed this Jul 19, 2024
@romainmenke
Copy link
Member

romainmenke commented Jul 19, 2024

Commonjs is supported.
Only types for commonjs are not.

If you get a warning about types, they you are actually generating commonjs.

We can't generate types that are correct for both esm and cjs.
This has been explained in detail in the linked issues.

So this is not a compat issue, commonjs works.
Please don't imply that we are intentionally breaking things when we go above and beyond to provide the best possible software, for free.

You just don't get types, which is not blocking, you can silence the warning about the types. We also have API docs to aid you in using these packages.

@ssttevee
Copy link
Author

You're right, I did not fully understand the situation and I apologize. However, I would like to add my data point.

For reference, my project does not use commonjs, but generates esm bundles with what I suppose is commonjs configurations in both my package.json and tsconfig.json. I could be using it completely wrong, but I believe this setup was rather common near the introduction of esm in nodejs.

The bundler automatically includes the esm source if available and falls back to cjs with a wrapper. We have not run into any commonjs issues related to type definitions using this setup.

I was able to rollback to v2.2.1 for the type definitions.

@romainmenke
Copy link
Member

For reference, my project does not use commonjs, but generates esm bundles with what I suppose is commonjs configurations in both my package.json and tsconfig.json

🤔 If your project really is using and generating esm, then you should have working types.
Can you share your configuration?

Or even better submit a PR with a minimal repro?

We have some e2e tests to ensure that TypeScript works, for example this one : https://github.com/csstools/postcss-plugins/tree/main/e2e/typescript/esm

So either your project isn't generating esm, which really is easy to do by accident, or we do have a bug somewhere.

@ssttevee
Copy link
Author

There is actually no problem with the bundling. My deployment environment does not support commonjs, so the generated bundle is definitely esm.

The only problem is the tooling. Because my moduleResolution is not node16, typescript doesn't read the exports and the type checking fails. I'm fairly certain this is an artifact of the bare minimum effort to make a pre-esm project to target an esm environment.

I don't think I'll have time to build a minimal repro, but here is my tsconfig:

{
  "compilerOptions": {
    "allowJs": true,
    "target": "ES2017",
    "module": "esnext",
    "lib": [
      "es2020",
      "DOM",
      "WebWorker",
      "DOM.Iterable"
    ],
    "jsx": "react-jsx",
    "jsxImportSource": "@builder.io/qwik",
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "resolveJsonModule": true,
    "moduleResolution": "node",
    "esModuleInterop": true,
    "skipLibCheck": true,
    "incremental": true,
    "isolatedModules": true,
    "outDir": "tmp",
    "noEmit": true,
    "types": [
      "node",
    ],
    "paths": {
      "~/*": [
        "./src/*"
      ]
    }
  },
  "include": [
    "src",
  ],
}

I have also tried using node16 module resolution, but that breaks more packages than it fixes.

@romainmenke
Copy link
Member

romainmenke commented Jul 19, 2024

Do you have "type": "module" in your package.json or are you using the .mts file suffix? If you don't do either TypeScript will still act as if you are writing commonjs.

@ssttevee
Copy link
Author

Nope, and that's exactly the problem.

I tried renaming the file to .mts but that does not seem to change the module resolution.

@romainmenke
Copy link
Member

https://www.typescriptlang.org/tsconfig/#moduleResolution

'node10' (previously called 'node') for Node.js versions older than v10, which only support CommonJS require. You probably won’t need to use node10 in modern code.

So you are still effectively using commonjs, which we don't have types for :)

So this definitely is an issue with your config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants