feat: improve sorting of imports/exports

[GeorgeTaveras1231]

Implement sorting for import and export fields.

1. Force relative position of known conditions, applying a sorting order from most-specific to least-specific.
2. Allow users to define custom conditions while minimizing order changes to custom conditions.
3. Sort paths alphabetically
4. Ensure paths with wildcard patterns are at the end

Fixes #294

Resources

• Webpack docs on known conditions
• "bun" condition
• Bun's "macro" condition"
• Nodejs's docs on conditional exports
• Winter CG Runtime Keys standard

[keithamus]

This seems reasonable to me but I'd like to get reviews from @jlarmstrongiv, @fisker, and @mrmckeb who all commented on the issue and so I imagine have opinions around this change.

[GeorgeTaveras1231]

Sounds good @keithamus. A question about the commit checks. The tests are failing on Node 12 and 14 because I'm using a relatively modern syntax (&&=). Those node versions are no longer maintained (in fact, only node 18 is being maintained at the moment) - and it would be a breaking change to stop supporting these old versions, but thought I'd ask you about your position on the subject. Are the builds an accurate representation of the node versions you intend to support?

[keithamus]

Those node versions are no longer maintained (in fact, only node 18 is being maintained at the moment) - and it would be a breaking change to stop supporting these old versions, but thought I'd ask you about your position on the subject. Are the builds an accurate representation of the node versions you intend to support?

They're quite out of date. No one has made a PR to update our supported versions (I don't author code for this repo any more, only steward PRs and releases). So that is to say, PRs welcome to update these!

Given the current version supports these older versions, I'd recommend changing this PR to avoid using the new syntax, so that we can release this as a feature release. Beyond that I'd then encourage you or others to make a PR cutting off support for Node.js <20 (given that 18 will be EOL in ~2 months) while also updating our build matrix to include 20, 22, 24 (and maybe lts for good measure so that we don't fall quite so behind). We can then release just this change as a major, and potentially do cleanup work (which might include updating dependencies and syntax) in follow up point releases.

[fisker]

As I understand, we don't need list all conditions, we only need care about types(fisrst) default(last) and module-sync(beforerequire). Am I correct?

[fisker]

We should get current order keys, check the conditions above, and move them to correct position, and sort with the new oredered keys.

[GeorgeTaveras1231]

As I understand, we don't need list all conditions, we only need care about types(fisrst) default(last) and module-sync(beforerequire). Am I correct?

@fisker That would be a simpler approach but I think the same issue applies to many of the conditions listed here (not all). In addition, I thought it would be a nice stylistic benefit to always group similar conditions (hence the condition groups).

But to elaborate on how the same issue applies to other conditions, imagine a scenario like this

{
  "import": "./index.esm.js",
  "require": "./index.cjs",
  "node": "./index.node.cjs",
}

In this example, the "node" condition has no effect because it is more specific than both "import" and "require" - node will generally use one of those conditions and always ignore the "node" one in this example.

[fisker]

In this example, the "node" condition has no effect because it is more specific than both "import" and "require" - node will generally use one of those conditions and always ignore the "node" one in this example.

Not true, --conditions flag can be used to make it work. I think it's better to not touch them, if someone request to change order of node, we can discuss later.

[GeorgeTaveras1231]

In this example, the "node" condition has no effect because it is more specific than both "import" and "require" - node will generally use one of those conditions and always ignore the "node" one in this example.

Not true, --conditions flag can be used to make it work. I think it's better to not touch them, if someone request to change order of node, we can discuss later.

@fisker I've linked the node.js docs multiple times I'm not sure if we are looking at the same information.

It says

Quote from Node.js docs

Node.js implements the following conditions, listed in order from most specific to least specific as conditions should be defined:

"node-addons" - similar to "node" and matches for any Node.js environment. This condition can be used to provide an entry point which uses native C++ addons as opposed to an entry point which is more universal and doesn't rely on native addons. This condition can be disabled via the --no-addons flag.
"node" - matches for any Node.js environment. Can be a CommonJS or ES module file. In most cases explicitly calling out the Node.js platform is not necessary.
"import" - matches when the package is loaded via import or import(), or via any top-level import or resolve operation by the ECMAScript module loader. Applies regardless of the module format of the target file. Always mutually exclusive with "require".
"require" - matches when the package is loaded via require(). The referenced file should be loadable with require() although the condition matches regardless of the module format of the target file. Expected formats include CommonJS, JSON, native addons, and ES modules. Always mutually exclusive with "import".
"module-sync" - matches no matter the package is loaded via import, import() or require(). The format is expected to be ES modules that does not contain top-level await in its module graph - if it does, ERR_REQUIRE_ASYNC_MODULE will be thrown when the module is require()-ed.
"default" - the generic fallback that always matches. Can be a CommonJS or ES module file. This condition should always come last.

The conditions import, require and node are all managed internally by nodejs - I don't think your comment about being able to specify them via the --conditions flag is true.

I can give you plenty of other examples of why more of these conditions have to be sorted.

For example:

• types must come before node
  • because tsc sets the node condition when targeting node runtime
• types must come before browser
  • because tsc sets the browser condition when targeting browser runtime
• webpack (or any bundler condition) must come before module
  • because otherwise, bundlers that use the module condition will ignore their own condition (webpack, etc) meant to override specific things for that bundler
• browser (or any target runtime condition) must come before module
  • because otherwise bundlers targeting a browser environment will ignore the browser condition meant to override the code with browser specific APIs

There are SOME conditions that are being sorted that have less consequences if left unsorted, but I think there is value is keeping conditions that are classified similarly close to each other - for example style, sass, etc.

Because these conditions are classified as "reference syntax" conditions, they are likely to suffer from the same issues and require the same treatment as other "reference syntax" conditions (like module, import, require) -- they are less specific than the conditions above them (bundlers, environments, target environments) and using them higher in the conditions object risks disabling code for an explicitly configured environment on the consumer side.

---

EDIT: More concrete examples supporting the need to sort

Example: Package exposes sass - with a custom sass file for webpack

❌ INVALID: webpack override is ignored when compiled by webpack

{
  "sass": "./main.scss",
  "webpack": {
    "sass": "./main.webpack.scss"
  }
}

✅ webpack override is used - falls back to normal SASS conditions in other environments

{
  "webpack": {
    "sass": "./main.webpack.scss"
  },
  "sass": "./main.scss"
}

Example: Package exposes electron binding with an override for development environment

❌ INVALID: Using a bundler targeting electron with a configured development environment, ignores the development condition

{
  "electron": "./main.electron.js",
  "development": {
    "electron": "./main.electron.development.js"
  }
}

✅ VALID: development override will be used

{
  "development": {
    "electron": "./main.electron.development.js"
  },
  "electron": "./main.electron.js",
}

Example: Package exposes entrypoint for bun runtime environment as well as dual module formats (cjs + esm)

❌ INVALID: Bun uses the import condition, ignoring the entrypoint meant for bun

{
  "import": "./main.mjs",
  "require": "./main.cjs",
  "bun": "./main.bun.ts"
}

✅ VALID: bun uses the main.bun.ts file meant for the bun runtime

{
  "bun": "./main.bun.ts",
  "import": "./main.mjs",
  "require": "./main.cjs",
}

Example: Package exposes code for a React Server Components (RSC) framework (like Next.js) as well as normal React components

❌ INVALID: Invalid, react-server condition is ignored because framework bundlers use the module condition

{
  "module": "./main.js",
  "react-server": "./main.rsc.js"
}

✅ VALID: Frameworks use code intended for RSC implementation

{
  "react-server": "./main.rsc.js",
  "module": "./main.js"
}

[GeorgeTaveras1231]

@fisker First off - thanks for taking the time to review this PR.

My impression is that we are not aligned about what this is solving, so I'm summarizing our positions so that we can focus the conversation and move forward.

TLDR:

• I now agree with moving module-sync above require.
• My primary goal is to avoid dead-code paths (unreachable conditions) - Can we align that this approach solves that?

---

Your position seems to be (correct me if I'm wrong):

1. Sort conditions based on the status quo: most devs using the module-sync condition use it to move away from the cjs format because older node versions don't support module-sync
2. Avoid sorting any other condition because it feels unnecessary.

To clarify my position:

Sorting should follow guidelines of "more specific" to "least specific" with the following goals:

1. Prioritizing conditions that are explicitly chosen by consumers of the package (by choosing targets/environments for the program)
2. Falling back to conditions that are inherent by the nature of the file (syntax and module semantics).
3. Minimize creating dead-code paths (conditions that will never be met - like the examples I showed above).

About module-sync:

I am coming around to your original idea to move it above require. The Node.js docs seem a little misleading but my last interpretation is that module-sync will only affect the behavior of require(ESM) in practice. Even though this will create dead-code paths in newer node versions (which will totally ignore the require condition - I'd commit to having the following order

• import
• module-sync
• require

I hope this helps anchor our conversation. 🙏🏽

[GeorgeTaveras1231]

Update:

• I updated the order of module-sync to be after import and before require.
• I've also made some fixes to the relativeOrderSort to properly respect the original order of custom keys.
• I've captured more standard conditions from the Winter CG standard promoted by Node.js which includes many conditions for edge runtimes. Edge-Runtimes go above target environments as they may be more specific versions of those environments (like a nodejs edge, or a deno edge). They also go above react-server, since some edges can also run react-server apps.

[fisker]

Yes, I still believe we should focus on solve problems mentioned in #294 , I don't want this PR break their conditions, and I hope PR as small as possible. Further improvements can be done if someone bring it up later.

• Move types to top, this should be our primary goal, it's all the issue about. TS documented it should be the first.
• Move default to bottom, it feels obvious to me. (you seems agree too, if not we can also leave this alone)
• About module-sync, we should either check if it's above require or leave it alone, not really care because it still new to most developers, but I don't want it moved under require if I already have it above require.

For libs, most time there are only the most popular conditions used, I feel unnecessary to care about other conditions.
For apps, too many tools work differently and most of them are configable about how to resolve. I feel too risky.

[GeorgeTaveras1231]

@fisker

• I'll go ahead and force the types to the top - this seems reasonable.
• I already handled module-sync to move it above require.

The thing about #294 is that it is too narrow. I came to this repo to solve this problem because I maintain a monorepo with 100+ workspaces supporting conditions like types, module, require, import, browser, node, react-server, style, sass, development, test, production as well as some custom conditions; I usually have to manually audit these conditions to make sure there aren't any dead code paths, and when we miss a problem, it usually has a long feedback loop because we find out from consumers; or it may go on as an unnoticed bug.

I only tagged #294 because it was a related issue, but the issue is too narrow. I imagine that other maintainers of large complex projects with a lot of conditions will encounter the same issues I have and would benefit from this.

Also, the rules are very straightforward and widely applicable:

• sort from concrete to abstract (i.e. import before module-sync, browser before import)
• sort from configurable to un-configurable (i.e. development before module)
• sort from supersets to subsets (i.e. edge-light before react-server, react-server before node See related issue in next.js repo)

With a few nuanced exceptions like module-sync and types - this approach works and helps avoid a lot of problems.

I also believe that there is an unavoidable risk with this change. Even if you limit it to just types, module-sync and default, if these are not sorted correctly and a developer uses this tool to sort them, they will likely change the way the package is interpreted (causing breaking changes). tsc checks can start failing, code in newer versions of node that was previously dead-code will be stressed and can cause errors on the consumer.

I appreciate that you are being careful, but if I can't address my issues holistically, then I would prefer to find a solution with another package (my own or eslint-plugin-package-json)

[GeorgeTaveras1231]

Update:

I spoke with a wise man who clarified that you are likely resistant to the change because it has a large scope and prefer to incorporate the fixes in small increments.

If it seems like I'm being stubborn is because I'm looking for validation of my examples and my description of the problems I've encountered. So far, the rejection of the changes have just come off as dismissive to my own problems.

I'm okay with breaking up the PR, if we can find an agreement about incorporating all the other known conditions.

[GeorgeTaveras1231]

Not true, --conditions flag can be used to make it work. I think it's better to not touch them, if someone request to change order of node, we can discuss later.

Following up with this - I created a reproduction repo to show that the node condition has to be on top of import and require as well as the environment conditions (like development, test and production)

https://github.com/GeorgeTaveras1231/package-export-condition-errors/tree/main

[GeorgeTaveras1231]

Update:

I'm working out a proposal to handle this properly. Once I have the details figured out, I will open an issue to discuss it in detail.

[GeorgeTaveras1231]

Closing to allow @fisker to handle condition sort of types and default. Opened #350 which only sorts the path objects.