| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
//cc @nodejs/modules-active-members |
Sorry, something went wrong.
|
Object order isn't guaranteed for JSON, do we have any concerns about other languages/etc. that do not have Object ordering? |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm but likely want some WG approval since there has been back and forth on it
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
{
"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. |
Sorry, something went wrong.
|
Yes, numeric properties are a problem - we should likely then operate under the assumption that valid property names must start with a-z. |
Sorry, something went wrong.
|
@guybedford if folks wanted to tag to a specific year release of the language they might start with something like 2019 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
Or node versions like 13.2 |
Sorry, something went wrong.
|
'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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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". |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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". |
Sorry, something went wrong.
|
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..? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I've added a new docs commit here in 63266ca with the following main points:
Let me know if that wording resolves the concerns raised. |
Sorry, something went wrong.
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). |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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.
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"? |
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #31008 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>
PR-URL: #31008 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>
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
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
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
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; |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for catching the JavaScript'ism here - PR welcome, otherwise I will include a fix in my upcoming PR in this file.
Sorry, something went wrong.
PR-URL: #31008 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>
| Back | FazBrowse Home | New Git URL |
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:
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:
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