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

fix: package.json export map proper order #200

Closed
wants to merge 1 commit into from

Conversation

Hebilicious
Copy link

@Hebilicious Hebilicious commented Jun 12, 2023

Description

types should come after require and import to not override the map.

There's something wrong with https://publint.dev/rules#exports_types_should_be_first, the "correct" way to have an export map that works without errors is to do this :

  "exports": {
    ".": {
      "require": {
        "types": "./dist/module.d.cts",
        "default": "./dist/module.cjs"
      },
      "import": {
        "types": "./dist/module.d.mts",
        "default": "./dist/module.mjs"
      },
      "types": "./dist/module.d.ts",
      "default": "./dist/module.mjs"
    },
    "./*": "./*"
  },
  "main": "./dist/module.cjs",
  "module": "./dist/module.mjs",
  "types": "./dist/types.d.ts",
  "files": [
    "dist",
    "*.d.ts",
    "*.cjs",
    "*.mjs"
  ],

This gives all green for packages that emits both cjs and esm files => https://arethetypeswrong.github.io/

See example of a working map.

Linked Issues

bluwy/publint#46

Additional context

This is not very concerning as it's easy to override on my side, but I think it's an improvement. Feel free to close if you don't like it.

types should come after require and import to not override the map.
@Hebilicious Hebilicious marked this pull request as draft June 12, 2023 21:19
@Hebilicious Hebilicious changed the title package.json export map proper order feat: package.json export map proper order Jun 12, 2023
@Hebilicious Hebilicious changed the title feat: package.json export map proper order fix: package.json export map proper order Jun 13, 2023
@Hebilicious Hebilicious marked this pull request as ready for review June 13, 2023 10:01
@antfu
Copy link
Owner

antfu commented Jul 4, 2023

I will hold this on, until TypeScript and publint change their documentation to advocate the right usage.

@Hebilicious
Copy link
Author

Hebilicious commented Jul 4, 2023

I will hold this on, until TypeScript and publint change their documentation to advocate the right usage.

Sorry I think I linked the wrong publint issue, this one got fixed already bluwy/publint#47 👍🏽
There's no type errors with package.json like this.

@antfu
Copy link
Owner

antfu commented Jul 5, 2023

I still do not understand what's going on with it. TypeScript recommends to put it on the top, but not it's not a good practice? Why wouldn't TypeScript change their docs?

Copy link
Author

@antfu I think it needs to be on top, excluding nested conditions

good

    ".": {
      "require": {
        "types": "./dist/module.d.cts",
        "default": "./dist/module.cjs"
      },
      "import": {
        "types": "./dist/module.d.mts",
        "default": "./dist/module.mjs"
      },
      "types": "./dist/module.d.ts",
      "default": "./dist/module.mjs"
    },

bad because the require.types and import.types will be overwritten by types

    ".": {
      "types": "./dist/module.d.ts",
      "default": "./dist/module.mjs"
      "require": {
        "types": "./dist/module.d.cts",
        "default": "./dist/module.cjs"
      },
      "import": {
        "types": "./dist/module.d.mts",
        "default": "./dist/module.mjs"
      },
    },

Where are the Typescript docs about this ?

@antfu
Copy link
Owner

antfu commented Jul 5, 2023

But I guess the current config does not address the difference between top-level and nested order, right?

@Hebilicious
Copy link
Author

Hebilicious commented Jul 5, 2023

But I guess the current config does not address the difference between top-level and nested order, right?

Yes ! For now I'm using an override like this

  "overrides": [
    {
      "files": [
        "package.json"
      ],
      "parser": "jsonc-eslint-parser",
      "rules": {
        "jsonc/sort-keys": [
          "error",
          {
            "pathPattern": "^exports.*$",
            "order": [
              "require",
              "import",
              "types",
              "default"
            ]
          }
        ]
      }
    }
  ]

But this isn't ideal as you can have any string as a named export, not just require/import. Ideally it should understand the nesting level and that types are always at the top, but the nested level are above root exports.

@zanminkian
Copy link
Contributor

zanminkian commented Jul 14, 2023

Personally, types field in exports is really hard to understand, and has bug. See microsoft/TypeScript#50762 and https://github.com/arethetypeswrong/arethetypeswrong.github.io. So banning types in exports will make your life easier. Just using types field in the root of package.json

@Hebilicious
Copy link
Author

Personally, types field in exports is really hard to understand, and has bug. See microsoft/TypeScript#50762 and arethetypeswrong/arethetypeswrong.github.io. So banning types in exports will make your life easier. Just using types field in the root of package.json

It looks like they're making progress in this Typescript issue. Slowly but surely we'll be able to fix the ecosystem.
Unfortunately if you want to release a cjs/esm packages that is backwards compatible with older version of node, you have to be very specific.

This proposed change prevent the linter from re-ordering the key in the wrong order.

@antfu
Copy link
Owner

antfu commented Sep 21, 2023

This PR conflicts with the #250 rewrite (sorry!). If you still want to push it, please create a new PR from the main branch. Thanks!

@antfu antfu closed this Sep 21, 2023
@Hebilicious
Copy link
Author

This PR conflicts with the #250 rewrite (sorry!). If you still want to push it, please create a new PR from the main branch. Thanks!

New refactor looks great ! Happy to open a new PR, but do you want to merge this change?
Personally I'm using my own eslint-config package (based on yours with overrides), so I'm fine either way.

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 this pull request may close these issues.

None yet

3 participants