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

module: implement logical conditional exports ordering #31008

Closed
wants to merge 6 commits into from

Conversation

@guybedford
Copy link
Contributor

guybedford commented Dec 17, 2019

This updates the ordering of matching for conditional exports to apply in logical order, as opposed to an invisible priority order of the conditions set by the resolver itself. This effectively makes the logic line up with the intuitive model that @sokra proposed in nodejs/modules#452 (comment).

The main reason is that with many conditions, making sense of what will match becomes almost impossible. Consider for example:

{
  "exports": {
    "browser": "./main-browser.js",
    "production": "./main-production.js",
    "development": "./main-dev.js",
    "node": "./main-node.js",
    "default": "./main.js"
  }
}

Strictly speaking, the resolver order of the above would be (assuming either core support for production / development, or through a custom flag): 1. production/development 2. browser/node 3. default.

Yet as a user setting the browser + production condition, there is no clear indication at all that "browser" will never be used, and "production" will always take priority. With this change, "browser" is chosen first whenever browser is enabled.

By moving to object order, the user is in full control of the matching, and can be sure their preferred target condition will be selected first.

The other change is with nesting, for example in:

{
  "exports": {
    "production": {
      "browser": "./asdf.js"
    },
    "default": "./x.js"
  }
}

Currently if in the "node" + "production" environment, trying to load the package will throw entirely because we greedily match the "production" condition, and then fail to match the browser condition and then bail.

With this PR, the failure on the "production" condition will still fall back to trying the "default" condition, providing a nicer fallback workflow.

The examples of production / browser are all hypothetical to explain the semantics - the conditions supported remain the same under this PR.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 17, 2019

Object order isn't guaranteed for JSON, do we have any concerns about other languages/etc. that do not have Object ordering?

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 17, 2019

Object order isn't guaranteed for JSON, do we have any concerns about other languages/etc. that do not have Object ordering?

There's definitely some potential issues here but I think even where languages don't preserve object key order by default, they should expose some API that does. E.g. for Python I found this: https://stackoverflow.com/a/43789495

@bmeck
bmeck approved these changes Dec 17, 2019
Copy link
Member

bmeck left a comment

lgtm but likely want some WG approval since there has been back and forth on it

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 17, 2019

@jkrems I can certainly find libraries that do some kind of insertion ordering, but the question is more about places that are not ordering them. See things like https://github.com/FasterXML/jackson for the JVM

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 17, 2019

@bmeck I assume when using jackson, you could serialize into/from a LinkedHashMap<> which should preserve order..? I haven't verified that myself though. Just like with Python I assume the danger here is that people serialize into the wrong data structure and less that the parser itself is doing some sort of ordering. Actually, it looks like Jackson may default to using LinkedHashMap<> and preserves order.

@weswigham

This comment has been minimized.

Copy link

weswigham commented Dec 17, 2019

{
  "exports": {
    "production": "./main-production.js",
    "0": "./main-dev.js",
    "1": "./main-node.js",
    "default": "./main.js"
  }
}

? If my knowledge of how js runtimes handle numeric-named keys is correct, the order for the above will not be the order listed.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

Yes, numeric properties are a problem - we should likely then operate under the assumption that valid property names must start with a-z.

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 17, 2019

@guybedford if folks wanted to tag to a specific year release of the language they might start with something like 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

@MylesBorins in that case we would need to ensure they make such a scheme es2019 or similar so as not to start with a numeric character.

@weswigham

This comment has been minimized.

Copy link

weswigham commented Dec 17, 2019

Or node versions like 13.2

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Dec 17, 2019

'13.2' is treated like a string property, not an indexed property. only ints in the range of 232-1 are treated as indexed properties (in the context of object key ordering)

the behaviour of this change seems very brittle to me, both in terms of js behaviour and other language behaviour. if we want order i'd suggest using an array.

@sokra

This comment has been minimized.

Copy link

sokra commented Dec 18, 2019

I haven't reviewed the code, but the PR text seems good to me. It should be easy to avoid indexed conditions making sure to always prefix them. E.g. es2019 instead of 2019, or node13 instead of 13.

Since JS specs insertion order I find it safe to rely on it. Even if some parsers in other languages don't use insertion order, it's easy to create a parser in that language that uses insertion order, since parsing JSON isn't very complex anyway.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 18, 2019

I'm starting an ecosystem audit for tools that do manipulate key ordering in JSON to check if any compatibility surprises might be waiting, there are many https://www.npmjs.com/search?q=sort%20json . We should be checking for usage within heavily used tools and creation of build artifacts that might affect package.json files. Per the meeting we had today, if problems exist, it may be tenable to define the ordering not to be on object insertion order, but instead on sorted order.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 18, 2019

We already now that the latest version of sort-package-json (200k) downloads currently sorts the exports subkey alphabetically (as it does for all unknown subkeys afaict).

For this research: I assume that finding examples doesn't block us from using order but it will give us an idea of the cost to adopt the feature by packages currently using those tools. Does that sound fair? E.g. the package above does sorting but already special-cases fields and would just have to start also special-casing exports.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 18, 2019

For this research: I assume that finding examples doesn't block us from using order but it will give us an idea of the cost to adopt the feature by packages currently using those tools. Does that sound fair? E.g. the package above does sorting but already special-cases fields and would just have to start also special-casing exports.

Any sort of requested changes would depend on ecosystem cost. If the cost is minor/insignificant I don't see any current objections. I think problems from this tooling phased transformation are non-trivial to root out but we can at least see if any major usages would impede us. However, I would not that my bar for compatibility is likely higher than others; and if the tooling/linting/etc. is impactful I'd seek to have alphabetical ordering rather than altering all the tooling.

{
   "exports": {
     "node": {"import": "...", "require": "..."},
     "default": "..."
   }
}

vs

{
   "exports": {
     "default": "...",
     "node": {"import": "...", "require": "..."}
   }
}

Does not appear significant enough to my knowledge to state we should change tooling rather than the conditional exports ordering. If another sorting issue arises that isn't alphabetical that would also be a contingency, but I haven't seen a linting rule or tool that does any manipulation except alphabetizing.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 18, 2019

Any sort of requested changes would depend on ecosystem cost.

I think we should point out here that this cost only applies to packages that use very specific tools to make changes to their package.json and only if they opt to use exports in their own package. I don't think we should worry about people who do arbitrary rewrites of package.json files in their entire node_modules tree for example. So unless npm init etc. are doing it, I think we already greatly scoped down the potential cost. There's a difference between "releasing this will break users of this tool" and "actively opt-ing into this feature may require additional work from some users".

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 18, 2019

@jkrems the tools are not necessarily in the packages themselves, anything doing publishes on people's behalf and linting etc might not be directly under their control.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 18, 2019

That still falls into the distinction of “does it stop rollout or does it stop adoption”. E.g. if I adopt it in my package that I publish and it works in my toolchain, I wouldn’t consider it blocking the rollout unless we know of things where it stops consumption of the package.

If it only hinders active adoption by a group of maintainers that’s orders of magnitude less severe as if it would stop rolling out packages to certain users (or even all users). That’s the distinction I’d like to make. I’m not claiming that only one or the other exists.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 18, 2019

I wouldn’t consider it blocking the rollout unless we know of things where it stops consumption of the package.

I think this is where we diverge. If people are wishing to sort their package.json I don't see large enough gains from having being in object insertion order vs just sorted alphabetically to warrant forcing object insertion order.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 19, 2019

I think this is where we diverge.

I don't quite understand how we diverge. Your example is about people sorting their own package.json files (or having them sorted during their own publish process). That feels like it's clearly in the realm of "blocks adoption of this field by maintainers" and not "blocks rollout of this field to the ecosystem".

An example of "blocks rollout" would be if artifactory's API would mutate the order of (nested) object keys in package.json data. Any package using conditional exports would be unable to be rolled out to consumers using artifactory. Which is the point where the issue turns from "some maintainers may have to adjust their workflow if they want to use this feature" into "a maintainer may accidentally break bigger parts of the ecosystem by adopting the feature".

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 19, 2019

Btw - I'm not implying a specific bar to be set here. I'm not saying "maintainers need to adjust their workflow" is definitely acceptable. I'm saying that there's a difference between those two kinds of issues. So "we diverge" would mean that to you those two kinds of issues are of the same magnitude. I don't think that's what you actually meant to say though - unless it is..?

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 19, 2019

So "we diverge" would mean that to you those two kinds of issues are of the same magnitude. I don't think that's what you actually meant to say though - unless it is..?

They are of potentially similar magnitude to myself, yes. Not all workflows give full control of all part of your build and publishing infrastructure including my own at work where we use an automated configurable build system and tools like lerna.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 19, 2019

I've added a new docs commit here in 63266ca with the following main points:

  1. Condition names cannot be numbers (to avoid index ordering)
  2. Condition names cannot start with a dot.
  3. Also to include a note that we may define new conditions / guidance for conditions in future.

Let me know if that wording resolves the concerns raised.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 19, 2019

Not all workflows give full control of all part of your build and publishing infrastructure including my own at work where we use an automated configurable build system and tools like lerna.

But you can just... not add conditional exports to your project or work through any issues with adding it at your own pace. The reverse is: You cannot possibly pull in this critical security fix from a dependency because it started using conditional exports and it breaks your environment. That's the difference between "my project keeps working unless I really want to use a shiny new feature" and "my project got broken by something out of my control and I can't unbreak it unless I retool my workflow". I don't quite understand how the former scenario is a "drop everything" bug (the latter pretty clearly is to me).

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Dec 19, 2019

@jkrems I'm not stating we shouldn't land conditional exports/nesting. I'm stating that the ordering by which keys are prioritized might need to be changed from insertion ordering to sorted ordering.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Dec 19, 2019

Then maybe that's where we are diverging. :) I am saying that we should potentially not ship key-order-conditionals if we find out that a tool like artifactory is broken by it.

I'm stating that the ordering by which keys are prioritized might need to be changed from insertion ordering to sorted ordering.

Just to clarify: In that solution evaluation order would be alphabetical? E.g. in a variant of your previous example it would check browser, then default, then node (or the reverse if descending)? I'm not sure how useful that behavior would be given how frequently that wouldn't match the desired priorities. Faced with that as a candidate for an order, I would rather go with "objects may only have a single key" and say that order is always provided by a wrapping array.

P.S.: "Change to sorted ordering" sounds like it's different from the current behavior. But right now we do prioritize based on a sort order. It's just not alphabetical but based on a custom comparison that enforces a well-known order of keys. So maybe what you mean is - relative to the current master branch behavior - "the order should remain prioritized based on a sorted ordering"?

src/module_wrap.cc Outdated Show resolved Hide resolved
@guybedford guybedford force-pushed the guybedford:exports-ordering branch from ab70890 to 9bfdebf Jan 7, 2020
@nodejs-github-bot

This comment has been minimized.

guybedford added a commit that referenced this pull request Jan 7, 2020
PR-URL: #31008
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
guybedford added a commit that referenced this pull request Jan 7, 2020
PR-URL: #31008
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 7, 2020

Landed in 405e7b4.

@guybedford guybedford closed this Jan 7, 2020
MylesBorins added a commit that referenced this pull request Jan 16, 2020
PR-URL: #31008
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
codebytere added a commit that referenced this pull request Jan 20, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
MylesBorins added a commit that referenced this pull request Jan 28, 2020
PR-URL: #31008
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
if (!n->ToInteger(context).ToLocal(&cmp_integer)) {
return false;
}
return n_dbl > 0 && n_dbl < (2 ^ 32) - 1;

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 3, 2020

Member

I was building node with clang-11, and saw this:

[23/101] CXX obj/src/libnode.module_wrap.o                                             
../../src/module_wrap.cc:958:34: warning: result of '2 ^ 32' is 34; did you mean '1LL << 32'? [-Wxor-used-as-pow]
  return n_dbl > 0 && n_dbl < (2 ^ 32) - 1;                                            
                               ~~^~~~                                                  
                               1LL << 32

I think the warning as well as the suggestion of 1LL << 32 is correct, ^ is not exponentiation, its XOR (there are a few examples of this in the file). @guybedford

This comment has been minimized.

Copy link
@guybedford

guybedford Feb 3, 2020

Author Contributor

Thanks for catching the JavaScript'ism here - PR welcome, otherwise I will include a fix in my upcoming PR in this file.

BethGriggs added a commit that referenced this pull request Feb 6, 2020
PR-URL: #31008
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.