Incorrect Bundling Behavior

[crutchcorn]

Per jsdom/cssstyle#182, I noticed this package doesn't appear to be bundling proper ESM and CJS assets as-likely intended.

I'd love to submit a PR to fix this using TanStack Config, a project of mine and the rest of TanStack Team's that allows ESM and CJS configuration with very little configuration.

Some of the major changes that would be made would be:

• PNPM instead of NPM
• Moving .js files to .ts
• Replacing testing config with Vitest (should be faster)
• Custom ESPlugin replaced with one from TanStack Config

However, it should lead to a much more stable publishing platform and faster iteration on code

Would you be okay with this? I'd like to unblock this quickly, as it appears to be impeding many many JSDom projects looking to refresh their lockfiles.

[asamuzaK]

What is incorrect?

[asamuzaK]

To be clear, there is nothing wrong with the css-color itself.
The issue with require is that test runners like jest and vitest don't read the exports field in package.json correctly.

"type": "module",
"exports": {
  "import": "foo.js",
  "require": "foo.cjs"
}

jest and vitest both do not fully assume like the case above, if there was no exports.default or no exports.node (that they set as the default).
Note that exports.default or exports.node are optional, not mandatory, and the order written in the exports fields has an effect.
Ref: https://nodejs.org/api/packages.html#conditional-exports

[aryaemami59]

@asamuzaK Can you clarify your statement? Are you saying vitest and jest don't usually look at package.json exports?

[asamuzaK]

They do read exports, but not picking exports.require for some reason.
They do pick exports.default (at least vitest, not yet confirmed with jest), so I added that.

[aryaemami59]

@asamuzaK Either way, @crutchcorn is correct, @asamuzaK don't take this the wrong way, but this package is misconfigured:

There is no .gitignore file which should be there to prevent node_modules (and the dist) folder from being committed into the repo.

We have a top level types field but no types field in package.json#exports which causes the types to not be resolved correctly. Also We're missing a top-level main field which is always good to have as a last resort in case there is still some bundler/testing-tool that doesn't look at the exports field. There are other minor issues which I'm sure in some parts are responsible for causing issues. I'd be happy to help resolve some of them for you.

[asamuzaK]

There is no .gitignore file which should be there to prevent node_modules (and the dist) folder from being committed into the repo.

Done in #37

We have a top level types field but no types field in package.json#exports which causes the types to not be resolved correctly.

I have a question out of curiosity: as far as I know, cssstyle also doesn't provide types, so why does its dependent package, css-color, need types?

Anyway, let's figure out how to provide *.d.ts to cjs.
Maybe postbuild: tsc ... should do?
I'll investigate further.

Also We're missing a top-level main field which is always good to have as a last resort in case there is still some bundler/testing-tool that doesn't look at the exports field.

I'm a bit confused.
exports was added way back in Nodejs v12, do we really still need main?

There are other minor issues which I'm sure in some parts are responsible for causing issues. I'd be happy to help resolve some of them for you.

Welcome.
Please file new issues.

[crutchcorn]

@asamuzaK one of the ways you can supply TS files for CJS properly is to have .d.cts files alongside duplicated .d.ts files for ESM

[aryaemami59]

@asamuzaK Yes, and yes. The best way to go about this is let @crutchcorn submit a PR and resolve the issue, you can review it, and any part of it you don't like, we'll figure out what to do with next. @crutchcorn is a seasoned library maintainer and he wants to contribute and this is an open source project (as in you're not alone, you don't have to do it all by yourself), so let him. He is halfway done already and it's looking really good, so let'e holdout to see what the end product is going to be.

[asamuzaK]

one of the ways you can supply TS files for CJS properly is to have .d.cts files alongside duplicated .d.ts files for ESM

Thanks, but how?
Especially if there is a way from *.min.cjs would be great.

I'm not opposed to migrating to TypeScript, but only if there's something that can't be achieved without migrating to TypeScript.

[asamuzaK]

re require issue, I'm waiting someone to confirm #36 with jest.