loader: fix package resolution for edge case

[dygabo]

this commit solves a regression introduced with PR-40980.
if a resolve call results in a script with .mjs extension the format
is automatically set to module. This avoids the case where an additional
package.json in the same directory as the .mjs file would declare the
type to commonjs

the PR also contains a new test that covers this functionality.
It fixes the issue observed with #41167 during backport of #40980

relevant comment for this situation: #41167 (comment)

@nodejs/loaders

the issue was that for the case of yargs the type was solved wrongly to commonjs because of: this when importing yargs/helpers.
That happened because package.json had priority over the extension. With this PR the package.json information only applies if the extension of the script is .js

Refs: #40980
Refs: yargs/yargs#2068

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[tniessen]

also cc @JakobJingleheimer

[aduh95]

Can we add a test case where outer package.json has "type": "module" and inner package.json has "type": "commonjs" for completness? Also I'd like to see what happens when there is a query string or a hash is added to the "exports" url (e.g.: "exports": "./index.mjs?key=val"), when the file extension is .cjs, and when the file extension is something other (e.g.: .ts).

[dygabo]

@aduh95 : please consider the rewrite based on your comments and on the existing implementation of defaultGetFormat as it covers also --experimental-specifier-resolution. : 6f7d871
I will prepare also some additional tests for the remaining cases

[aduh95]

I think we should go into the try block only if the extension is .js, otherwise we already know the format (or we know we can't know the format).
We should ensure the experimental warning is emitted only once.

[dygabo]

pushed again based on review comments: 435dc9b. Please review.

we now have 6 test variations including those @aduh95 proposed.
one test scenario missing though: the one with a query string in the form "exports": "./index.mjs?key=val". Please provide an example and the expected behaviour in this context and I will prepare a test for that as well.

A real-world example using that feature would also be fine.

[aduh95]

one test scenario missing though: the one with a query string in the form "exports": "./index.mjs?key=val". Please provide an example and the expected behaviour in this context and I will prepare a test for that as well.

I meant having the "exports" field in package.json equals to "./index.mjs?key=val" – so with a query string in the URL. It would also be interesting to have one with a hash string ("./index.mjs#key") and – why not – both ("./index.mjs?key=val#key"). The expected behavior would be for the query string and the hash part to be completely ignored to define the format value. Let me know if you need more info.

[danielleadams]

@dygabo this also didn't land cleanly on 16.x (with #40980) - do you mind backporting?

[dygabo]

@danielleadams : also for this one I will look into it.

[Flarna]

@dygabo I recommend a single backport PR for both as we either want both or nothing.

see here to get some hints how a backport PR should look like.