loader: return package format from defaultResolve if known

[dygabo]

This is a proposed modification of defaultResolve to return the package
format in case it has been found during package resolution.
The format will be returned as described in the documentation:
https://nodejs.org/api/esm.html#resolvespecifier-context-defaultresolve
There is one new unit test as well:
test/es-module/test-esm-resolve-type.js

Please consider this PR as a discussion base if this feature is
correctly implemented.
The approach was to only return the format from defaultResolve if
a package.json file has been found and parsed.
All other cases only return url as before.

All unit tests are green, the new test passes as well.

The modification for recomputing the format type found in
lib/internal/modules/esm/load.js is a temporary solution for the
make test because highlight.js seems to be a dual package but
declares type: commonjs in its package.json:
https://github.com/highlightjs/highlight.js/blob/main/package.json#L35

[aduh95]

@nodejs/loaders

[GeoffreyBooth]

The modification for recomputing the format type found in lib/internal/modules/esm/load.js is a temporary solution for the make test because highlight.js seems to be a dual package but declares type: commonjs in its package.json

If the goal is to return the format “if known,” this edge case strikes me as a clue that perhaps the logic here is wrong or incomplete; that maybe the format isn’t actually known for certain conditions.

[dygabo]

The modification for recomputing the format type found in lib/internal/modules/esm/load.js is a temporary solution for the make test because highlight.js seems to be a dual package but declares type: commonjs in its package.json

If the goal is to return the format “if known,” this edge case strikes me as a clue that perhaps the logic here is wrong or incomplete; that maybe the format isn’t actually known for certain conditions.

my interpretation here was that maybe the package.json of highlight.js should not define the type in its package.json since it is a dual package and commonjs is the default type in node anyway. as I am rather new to node I don't know if this conclusion is correct.

is that scenario correct from the package implementation side? removing the "type": "commonjs", line from package.json of highlight.js solves it.

for the case:
main package.json contains:

  "type": "commonjs",
[...]
  "exports": {
    ".": {
      "require": "./lib/index.js",
      "import": "./es/index.js"
    },

and es/package.json contains:

"type": "module"

is in this case the correct result "module" in case of import? If that is the case it might be that there is a bug in defaultResolve for this case even without the changes in this PR since it solves to "commonjs" (I could look into it if needed). that is not sent out and load recomputes based on path and gets to the correct "module" result.

please let me know how to proceed.

[dygabo]

I looked a bit more into this and I think I found the part of the code where this happens:

• getPackageConfig creates an entry in packageJSONCache including main and type that are read from the main package.json file (in the specific testcase: ./lib/index.js and commonjs respectively).
• since the package.json contains an "export" section, packageExportsResolve gets called and this solves correctly the main script to be in the specific case ./es/index.js but it does not update the package type in the cache with the value in the ./es/package.json. In fact the code in packageExportsResolve does not look for the existence of a "deeper" package.json

I attach here a screenshot of the callstack where the script resolution happens because I think it explains more than I can do in text form:

if I find a feasible solution I will try to make a proposal, otherwise kindly waiting for input on this.

[aduh95]

my interpretation here was that maybe the package.json of highlight.js should not define the type in its package.json since it is a dual package and commonjs is the default type in node anyway. as I am rather new to node I don't know if this conclusion is correct.

The "type" field in the package.json is useful only to determine how to parse .js file. When loading a .js file, node has to look for the "closest"¹ package.json to know if it has a "type" set to "module", otherwise it's parsed as CJS.

Node.js docs recommends to always set the "type" field, regardless if the package is dual or CJS-only:

node/doc/api/packages.md

Lines 80 to 84 in e31d1cb

	Package authors should include the [`"type"`][] field, even in packages where
	all sources are CommonJS. Being explicit about the `type` of the package will
	future-proof the package in case the default type of Node.js ever changes, and
	it will also make things easier for build tools and loaders to determine how the
	files in the package should be interpreted.

Footnotes

1. the one in the same directory, or one of the parent directories, which may or may not be the same package.json that defines the "exports". ↩

[dygabo]

The "type" field in the package.json is useful only to determine how to parse .js file. When loading a .js file, node has to look for the "closest"1 package.json to know if it has a "type" set to "module", otherwise it's parsed as CJS.

Node.js docs recommends to always set the "type" field, regardless if the package is dual or CJS-only:

node/doc/api/packages.md

Lines 80 to 84 in e31d1cb

	Package authors should include the [`"type"`][] field, even in packages where
	all sources are CommonJS. Being explicit about the `type` of the package will
	future-proof the package in case the default type of Node.js ever changes, and
	it will also make things easier for build tools and loaders to determine how the
	files in the package should be interpreted.

Footnotes

1. the one in the same directory, or one of the parent directories, which may or may not be the same package.json that defines the "exports". ↩

Thanks for the tips. I updated the PR with one commit considering your input. I also added an additional testcase to test/es-module/test-esm-resolve-type.js to reflect the situation with the dual package and the additional package.json file.

All the tests are green and I look forward to hearing your thoughts on this.

[bmeck]

Seems fine to me pending, a comment in the test case explaining what is going on regarding 2 package.json files might help though since it is rather hard to read.

[dygabo]

Seems fine to me pending, a comment in the test case explaining what is going on regarding 2 package.json files might help though since it is rather hard to read.

I added now a comment to the source file explaining the test scenario and also check the returned url value: 022a2eb

[aduh95]

I'm not sure I understand this change, it looks like the added .format property is not used anywhere, so is it actually useful? Why are we adding it?

[dygabo]

I'm not sure I understand this change, it looks like the added .format property is not used anywhere, so is it actually useful? Why are we adding it?

it actually has an effect on the behaviour of defaultLoad but more than that it might be of advantage for external loaders added with --loader to have this information in their implementation after calling the defaultResolve in the resolve hook.

[aduh95]

I'm not sure I understand this change, it looks like the added .format property is not used anywhere, so is it actually useful? Why are we adding it?

it actually has an effect on the behaviour of defaultLoad but more than that it might be of advantage for external loaders added with --loader to have this information in their implementation after calling the defaultResolve in the resolve hook.

Could you add a test that doesn't use --expose-internals that illustrate that? I think it would help me understand.

[JakobJingleheimer]

Cool, thanks for this!

Custom resolve hooks can already return an optional format property, so this makes sense to me. I think there wasn't a particular reason we did not already update defaultResolve() to do this—I think maybe we were planning to do in a follow-up (so the original PR, that was already getting quite large, could land) and @dygabo got to it first 🙂

Approach and goal look good to me. Just a few hygiene nits.

[dygabo]

I'm not sure I understand this change, it looks like the added .format property is not used anywhere, so is it actually useful? Why are we adding it?

it actually has an effect on the behaviour of defaultLoad but more than that it might be of advantage for external loaders added with --loader to have this information in their implementation after calling the defaultResolve in the resolve hook.

Could you add a test that doesn't use --expose-internals that illustrate that? I think it would help me understand.

I cannot make a unit-test without the expose-internals in this case but I think it is explained by @JakobJingleheimer :
#40980 (review)

please let me know if that clarifies the use-case

[aduh95]

I cannot make a unit-test without the expose-internals in this case

Sure you can. For example, you could check that format and resolved are returned here:

node/test/fixtures/es-module-loaders/hooks-custom.mjs

Line 69 in 2cc7a91

return next(specifier, context, next);

[dygabo]

review comments solved with: 3894a73

[dygabo]

I cannot make a unit-test without the expose-internals in this case

Sure you can. For example, you could check that format and resolved are returned here:

node/test/fixtures/es-module-loaders/hooks-custom.mjs

Line 69 in 2cc7a91

return next(specifier, context, next);

thank you for the hint. If this would serve the purpose of you understanding the modification I would try to squeeze in some more tome for this next week. I thought the comment of @JakobJingleheimer explained it quite well and your question would be clarified by that.

Apart from that since I also am kind of on a tight schedule I would skip it as IMO the automated test with --experimental-loader does not bring additional value. That's because the existing testcase covers the modification as well.

Please all let me know your thoughts on this. If the additional testcase is needed I will find an evening in the upcoming week for that.