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" Field of Package.json Violates JSON Language Spec #37777

Closed
bdkjones opened this issue Mar 17, 2021 · 26 comments
Closed

"Exports" Field of Package.json Violates JSON Language Spec #37777

bdkjones opened this issue Mar 17, 2021 · 26 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@bdkjones
Copy link

bdkjones commented Mar 17, 2021

  • Version: 15.11.0
  • Platform: macOS 11.2.1
  • Subsystem: n/a

What is the bug?

The documentation for conditional exports in a package.json file is here: https://nodejs.org/api/packages.html#packages_conditional_exports

It specifies that the order of keys in the exports object matters within a package.json file. However, that is a direct violation of the JSON language specification. By definition properties of JSON objects are unordered: https://www.json.org/json-en.html

How do you reproduce the bug?

Consider this package.json file:

// package.json
{
  "name": "thing",
  "main": "./index.cjs",
  "exports": {
         "default": "./index.js",
         "import": "./index.js",
         "require": "./index.cjs"
  },
  "type": "module"
}

If we attempt to use this package via const thing = require('thing'), node will throw a huge error because it resolves to the default entry in exports, which is a *.js file.

This is because node is iterating the entries of exports in the order provided. This behavior is incorrect. Instead, node should load the entire exports object and then follow a standard approach to select the best match from the entries provided. In this case, since we are using require(), that might be:

  1. Does exports contain a require entry? If yes, use it.
  2. Does exports contain a node entry? If yes, use it.
  3. Does exports contain a default entry? If yes, use it.
  4. Throw an error.

In other words, the ORDER of the entries in package.json should be irrelevant. That should be an internal implementation detail of node, not a user-managed item.

What is the expected behavior?

Given an object for a "conditional export" that contains entries for require, import, node, and default, the ORDER of those entries in the package.json file MUST be irrelevant. Otherwise, package.json files are not JSON.

Why This Matters

The current approach requires a human to manually write the exports field. A user cannot, for example, use a JSON library to manipulate a package.json file because JSON libraries (correctly) assume that the order of properties on an object does not matter—they do not provide a way to manually specify that property X must come after property Y.

In my case, I was using a JSON library to remove extraneous fields and whitespace from a package.json file and this violation of the JSON spec took a few hours off my life: ai/nanoid#267

@targos targos added the module Issues and PRs related to the module subsystem. label Mar 17, 2021
@bdkjones
Copy link
Author

The good news is that changing this behavior introduces no breaking changes for users. Every manually-written package.json file will still work just as it did before. Plus, there will be one fewer foot-gun in node that requires a long explanation in the docs!

@targos
Copy link
Member

targos commented Mar 17, 2021

@nodejs/modules

@bdkjones
Copy link
Author

bdkjones commented Mar 17, 2021

Finally, I have seen "custom" entries in exports beyond the ones covered in the docs (import, require, node, default). If it is necessary to support a user-defined order for some reason, then default should be modified:

EITHER default is a string, as it is currently

OR

default is an array of strings to be considered for a match, in order:

 "exports": {
         "default": ["./one.js", "./two.js", "./index.js"],
         "import": "./index.js",
         "require": "./index.cjs"
  }

This provides user-control over the order for weird edge cases while still adhering to the JSON spec. This same "string OR array" approach could be applied to the other properties as well, if needed.

@aduh95
Copy link
Contributor

aduh95 commented Mar 17, 2021

default can already be an array of strings, as any property in "exports". I don't see what it has to do with "custom" user-defined entries though.

@benjamingr
Copy link
Member

benjamingr commented Mar 17, 2021

The actual spec is not json.org it's the ECMA-404 spec that actually allows this if I am reading correctly:

Screen Shot 2021-03-17 at 12 31 08

These are all semantic considerations that may be defined by JSON processors or in specifications defining specific uses of JSON for data interchange.

@benjamingr
Copy link
Member

Yes, if we check the ECMAScript specification it points to ECMA-404 to define JSON.

Under "Normative References":

ECMA-404, The JSON Data Interchange Format.
https://ecma-international.org/publications/standards/Ecma-404.htm

ECMA-404 https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf

So I am pretty convinced the spec is clear that we are allowed to assign significance to object key value without violating the spec we are implementing.


(I am not saying we should or whether or not it's a good idea)

@aduh95
Copy link
Contributor

aduh95 commented Mar 17, 2021

IMHO it would be worth considering if we can tweak the resolution algorithm to make the ordering optional. If someone wants to work on this, the relevant code is here:

} else if (typeof target === 'object' && target !== null) {
const keys = ObjectGetOwnPropertyNames(target);
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
if (isArrayIndex(key)) {
throw new ERR_INVALID_PACKAGE_CONFIG(
fileURLToPath(packageJSONUrl), base,
'"exports" cannot contain numeric property keys.');
}
}
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
if (key === 'default' || conditions.has(key)) {
const conditionalTarget = target[key];
const resolved = resolvePackageTarget(
packageJSONUrl, conditionalTarget, subpath, packageSubpath, base,
pattern, internal, conditions);
if (resolved === undefined)
continue;
return resolved;
}
}
return undefined;

@ljharb
Copy link
Member

ljharb commented Mar 17, 2021

The ordering is an important part of the design; given that package.json is matching node/v8's implementation precisely, it doesn't have to be interoperable - it only has to work reliably in node, which it does.

@ljharb
Copy link
Member

ljharb commented Mar 17, 2021

A user cannot, for example, use a JSON library to manipulate a package.json file because JSON libraries (correctly) assume that the order of properties on an object does not matter—they do not provide a way to manually specify that property X must come after property Y.

I'm not sure how that's accurate - a JSON library that does JSON.parse and JSON.stringify will have order preserved. It's only if they are sorting keys - something that the JSON spec does not prohibit but absolutely does not require - that you'd run into this problem.

Here's the node repl:

> JSON.stringify({ default: 1, node: 2 })
'{"default":1,"node":2}'
> JSON.stringify({ node: 2, default: 1 })
'{"node":2,"default":1}'
> JSON.parse('{"default":1,"node":2}')
{ default: 1, node: 2 }
> JSON.parse('{"node":2,"default":1}')
{ node: 2, default: 1 }

My JSON library, for example, doesn't suffer from this problem as far as I can tell: https://npmjs.com/json-file-plus

@jasnell
Copy link
Member

jasnell commented Mar 17, 2021

While JSON itself does not impose meaning to the ordering of fields within an object, applications using JSON are free to do so, and it's not a violation of the spec in any way.

@bmeck
Copy link
Member

bmeck commented Mar 17, 2021

I'd be -1 to this since as @ljharb points out this is a breaking change that would violate some of the design workflows for "exports". In addition the ordering of the CLI flag --conditions would need to be specified if we gave an absolute order to the key traversal. We did at one point look at arrays in nodejs/modules#452 if people want to dig through some history on why the current design was eventually chosen.

@bdkjones
Copy link
Author

bdkjones commented Mar 17, 2021

A few thoughts:

  1. The issue is JSON libraries outside of Javascript, where objects are represented by Dictionary types. The order of the key-value pairs is not preserved. In my case, that’s NSJSONSerialization—an Apple library in Swift and Objective-C. I’d expect the same behavior from libraries that are not using Javascript objects when parsing JSON.

  2. What is the downside to internalizing the order? The docs already clearly state that order: one of require or import followed by node and then default. Given this, why force users to manage this in the package.json file? Even if we do not care about non-javascript json libraries, what’s the drawback to removing a subtle way to cause bugs?

@bmeck
Copy link
Member

bmeck commented Mar 17, 2021

@bdkjones

The docs already clearly state that order: one of require or import followed by node and then default.

I am unclear on where the ordering is specified in the docs; it shouldn't be since in https://nodejs.org/dist/latest-v14.x/docs/api/packages.html#packages_conditional_exports it is stated: ""Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries.""

Given this, why force users to manage this in the package.json file?

Some of it is for human convenience, some of it is to disallow mis-ordering between tools (and features like --condition), another is for complex workflows.

For human convenience, specifying as an array was looked at in the issue linked above. The array is meaningless except to have an ordering of matching which as pointed out above is already provided in JS and in a few other situations in other languages (such as requiring type tags be at the start in some marshaling libraries). Per my personal preference and likely that of others hoping for easy DX in Node, that would be nice to have human convenience if it is not sufficiently inconvenient for a machine to handle.

To disallow mis-ordering, keeping the ordering in the schema itself was the only real way to do this. Tooling could diverge, add new conditions, etc. if the ordering was not in the JSON itself. This starts to lean back towards convenience.

Complex workflows are also hard to deal w/ and introduce some backtracking if we start using something like an array:

{
  "exports": {".": [
    {"node": {"require": "a"}},
    {"import": "b"}
  ]}
}

Currently, there is no backtracking to allow for cases where you don't want fall through. In the example above, without backtracking you can prevent someone from using import in node. With fall through it becomes harder to produce these error conditions. There were a few other examples from @jkrems back in the day about this (around December 2019?) but I can't seem to dig them up.

@bdkjones
Copy link
Author

@bmeck I still think this is a fragile design that implicitly ties package.json files to Javascript tooling only. However, to your credit, you’ve explained the details behind the choice well.

I’ll file an enhancement request with Apple Engineering to see if Swift’s JSON libraries can be updated to handle ordered property values. Someone should get around to responding in 3-7 years. The response will be “no”.

@DerekNonGeneric
Copy link
Contributor

@bdkjones, thank you for filing this issue. You bring up valid points. I have wanted to know the name of the data structure that preserves key order for this very reason. This is surely not your average JS Object type. It's not a Dictionary nor is it a SortedDictionary, which is alphabetical. Perhaps check if it exists in Swift? I checked C# to no avail. Trying to model this with types is a challenge, so I understand how this could be considered invalid from some perspectives.

@bmeck
Copy link
Member

bmeck commented Mar 17, 2021

These kinds of data structures are generally called Ordered Maps. There are a variety of ways to order maps, this particular one is in insertion order.

@bdkjones
Copy link
Author

@DerekNonGeneric Swift does have a class called “KeyValuePairs” that maintains order. The trouble is that the JSON libraries in Swift do not accept that class. They translate JSON objects to Dictionary types and the order of keys is random in a Dictionary.

To support what node is doing with exports, I’d have to write my own JSON parsing/writing library that maintains key order. If you use Apple’s library, you get an unordered Dictionary for an object—no ifs, ands, or buts.

While it’s clear that node’s behavior is not a technical violation of the JSON spec, it does rely on what is, for all practical purposes, non-standard behavior.

It’s just that the motivation to do anything about it is low because, as others have pointed out, this isn’t an issue for Javascript-based tools. It’s only an issue for other languages.

@DerekNonGeneric
Copy link
Contributor

Filing an enhancement request with Apple Engineering to see if Swift’s JSON libraries can be updated to use KeyValuePairs instead of Dictionary doesn't sound too outlandish to me. You might even be able to make a custom fork if the code is available.

@bdkjones
Copy link
Author

@derek-findthebrink I already know what they’ll say. Dictionary provides instant-lookup of any key. KeyValuePairs does not because the keys need not be hashable. The only way to do a lookup in a KeyValuePairs object is to iterate the keys until you find the one you’re after.

In 99.9% of cases, Dictionary is the better choice.

@bmeck
Copy link
Member

bmeck commented Mar 17, 2021

In general if you want a performant insertion order list you don't iterate key value pairs, Map for example in JS is ordered the same as Object properties. Keeping a separate table from your hash of the ordering and only messing with it based upon insertion/deletion is pretty snappy.

@benjamingr
Copy link
Member

Map for example in JS is ordered the same as Object properties

Map is insertion order and doesn't do the weird object properties "is this a numberic uint32-1 property or symbol" dance IIRC.

@aduh95
Copy link
Contributor

aduh95 commented Mar 20, 2021

FWIW I think we could make the algorithm accept nested arrays without being a breaking change:

{
    "name": "name",
    "exports": [
        [  "require",       "node",       "default"],
        ["./cjs.cjs", "./node.mjs", "./browser.mjs"]
    ]
}

This structure is easily turned back into an Object in JS:

if(Array.isArray(exports) && Array.isArray(exports[0]) && Array.isArray(exports[1])) {
  exports = Object.fromEntries(exports);
}

Of course I'm not saying we should do that, but I think that's a solution worth considering if keeping the order of keys is a big hassle outside of JS.
It's also a fairly easy solution to implement in user-land; if the OP is able to add a JS step to their build chain, they should be able to work around the issue.

@jkrems
Copy link
Contributor

jkrems commented Mar 22, 2021

It’s just that the motivation to do anything about it is low because, as others have pointed out, this isn’t an issue for Javascript-based tools. It’s only an issue for other languages.

To be a bit more concrete here: It's an issue for some older versions of Python and, now newly discovered, Swift/Obj-C. The popular JSON implementations across most languages either preserve insertion order or at least have a simple option to do it. When only looking at current versions, Swift/Obj-C is the odd one out that doesn't behave like other JSON implementations. And, as others have pointed out, that may means that it's arguably not a complete JSON implementation because the JSON spec does specify an order, kind of.

We already allow people to create exports fields that are 100% safe in the face of object key reordering:

{
    "name": "name",
    "exports": [
        [{"require": "./cjs.cjs"}],
        [{"node": "./node.mjs"}],
        // or just "./browser.mjs" because a string value is implicitly "default"
        [{"default": "./browser.mjs"}]
    ]
}

So if somebody wants to use Swift to format their package.json, they could use this form of exports and it just works™.

@GeoffreyBooth
Copy link
Member

So if somebody wants to use Swift to format their package.json, they could use this form of exports and it just works™.

I say we close as resolved?

@bdkjones
Copy link
Author

bdkjones commented Mar 23, 2021

@jkrems the issue isn't when I generate package.json files; it's when I consume them. Nobody structures them as you've proposed, which prevents Swift-based frameworks from parsing these files without potentially losing information.

(Although, semantically, what you propose above would have been the "correct" way to do this had it been enforced from the start. Now though, that ship has sailed and everyone omits the arrays.)

This isn't just old Python and Swift. Just based on some quick googling it appears that higher languages are not affected while lower ones are. For example, there's a bunch of SO threads about trying to maintain JSON key order in C++ JSON libraries: https://stackoverflow.com/questions/30837519/writing-in-order-to-jsoncpp-c

The JSON spec does not include ordering. It says: "You can assign an order to the properties of an object if you want, but that's your thing, not JSON's thing."

I'll go ahead and close the issue anyway because I'm just tired. Thanks.

@benjamingr
Copy link
Member

I'll go ahead and close the issue anyway because I'm just tired. Thanks.

I'm sorry it's tiring to you but this really isn't a bug in Node.js but in those frameworks :/

For what it's worth you can:

In either case opening a radar sounds like a reasonable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

10 participants