frehner / modern-guide-to-packaging-js-library

A guide to help ensure your JavaScript library is the most compatible, fast, and efficient library you can make.
GNU General Public License v3.0
1.44k stars 38 forks source link

`exports` advice is potentially wrong for types #35

Closed Zamiell closed 9 months ago

Zamiell commented 10 months ago

Hello, and thanks for the excellent guide.

When setting up my library, I followed this guide and copy-pasted the exports example provided here: https://github.com/frehner/modern-guide-to-packaging-js-library?tab=readme-ov-file#define-your-exports

However, after I published my library and then used publint, it flagged my package.json file for this: https://publint.dev/rules#export_types_invalid_format

Thus, I think that the posted example is wrong, and it should be updated to satisfy this lint rule.

frehner commented 10 months ago

Yeah, at the time this was posted, there was still a lot of confusion and discussion around this.

In any case, do you think updating things to more along their example would be better?

{
  "exports": {
    "import": {
      "types": "./index.d.mts",
      "default": "./index.mjs"
    },
    "require": {
      "types": "./index.d.cts",
      "default": "./index.cjs"
    }
  }
}
Zamiell commented 10 months ago

I'm a complete noob, so don't listen to anything I have to say. =p

With that said, aren't the "module" and "default" fields missing now? So shouldn't it be something like this?

  "exports": {
    ".": {
      "module": "./dist/index.mjs",
      "import": {
        "types": "./dist/index.d.mts",
        "default": "./dist/index.mjs"
      },
      "require": {
        "types": "./dist/index.d.cts",
        "default": "./dist/index.cjs"
      },
      "default": "./dist/index.mjs"
    }
  },
Zamiell commented 10 months ago

What I posted above passes publint, but it actually fails arethetypeswrong:

image

So I guess more tweaking is necessary.

Zamiell commented 10 months ago

Oh, nevermind, that was just because I was missing the base types. So the advice should actually be this:

  "exports": {
    ".": {
      "module": "./dist/index.mjs",
      "import": {
        "types": "./dist/index.d.mts",
        "default": "./dist/index.mjs"
      },
      "require": {
        "types": "./dist/index.d.cts",
        "default": "./dist/index.cjs"
      },
      "default": "./dist/index.mjs"
    }
  },
  "types": "./dist/index.d.ts",
frehner commented 10 months ago

Hmm, do you think we need the subobject with "types" for "module" and "default" as well?

Zamiell commented 10 months ago

As I said above, I have no idea, don't trust me. =p

Isn't "module" supposed to be just for webpack and/or hypothetical future bundlers that want to follow its arbitrary convention? Naively, I wouldn't expect webpack to ever need to resolve types; that would only be a type-checking thing via either TypeScript compiler itself or a tool like stc.

As for "default", I guess you could make an argument where there could be some hypothetical future where a use keyword is invented as a new kind of import, and then tsc needs to resolve the types for it. So then if someone had "types" in "default", then this would work out of the box without having to change anything, assuming that use was backwards compatible with a .d.mts file. So that's a reason to include it there.

But on the other hand, I'm not super happy about having to pollute my package.json file with all of these repetitive fields that all point to the same file anyway, and the added value here seems to be pretty marginal.

frehner commented 10 months ago

and the added value here seems to be pretty marginal.

Agreed.

Glad we had the discussion, and now it's preserved for future reference as well.

Would you like to create a PR to update it?