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

exports advice is potentially wrong for types #35

Closed
Zamiell opened this issue Dec 14, 2023 · 7 comments · Fixed by #36
Closed

exports advice is potentially wrong for types #35

Zamiell opened this issue Dec 14, 2023 · 7 comments · Fixed by #36

Comments

@Zamiell
Copy link
Contributor

Zamiell commented Dec 14, 2023

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
Copy link
Owner

frehner commented Dec 14, 2023

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

Zamiell commented Dec 14, 2023

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

Zamiell commented Dec 14, 2023

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

image

So I guess more tweaking is necessary.

@Zamiell
Copy link
Contributor Author

Zamiell commented Dec 14, 2023

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
Copy link
Owner

frehner commented Dec 14, 2023

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

@Zamiell
Copy link
Contributor Author

Zamiell commented Dec 14, 2023

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
Copy link
Owner

frehner commented Dec 14, 2023

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?

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 a pull request may close this issue.

2 participants