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

import.meta.resolve(…) is documented to return a string, but returns a URL object #49695

Closed
lgarron opened this issue Sep 18, 2023 · 8 comments · Fixed by #49698
Closed

import.meta.resolve(…) is documented to return a string, but returns a URL object #49695

lgarron opened this issue Sep 18, 2023 · 8 comments · Fixed by #49698
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@lgarron
Copy link

lgarron commented Sep 18, 2023

Version

v20.6.1

Platform

Darwin Germain.local 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:22:05 PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

https://nodejs.org/api/esm.html#importmetaresolvespecifier states:

Returns: <string> The absolute (file:) URL string for the resolved module.

The following prints a URL object:

echo "console.log(process.versions.node); console.log(import.meta.resolve('./rel'));" > /tmp/test.mjs
node /tmp/test.mjs

(import.meta.resolve('./rel')) instanceof URL also evaluates to true.)

How often does it reproduce? Is there a required condition?

N/A

What is the expected behavior? Why is that the expected behavior?

The documentation and the behaviour match.

All browsers and deno return a string, and it sounds like bun would also prefer this. I don't feel strongly myself, although I feel matching browsers would be less surprising if I was learning the API from scratch — it's always possible translate between formats if needed, but the ecosystem benefits from consistent type signatures in several ways.

See #48994 for a related discussion about API ergonomics.

What do you see instead?

The documentation and behaviour mismatch.

Additional information

No response

@GeoffreyBooth
Copy link
Member

Yes, this is a bug, thank you for flagging it. import.meta.resolve should return an URL string, similar to import.meta.url. cc @guybedford @aduh95 @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. labels Sep 18, 2023
@jimmywarting
Copy link

I think it should match deno / browsers behavior and return a url string.

@JakobJingleheimer
Copy link
Member

Ugh much as it pains me, it is spec'ed to return a url string. Not what anybody wanted, but it is what it is. Maaaybe something else coming.

@jimmywarting
Copy link

jimmywarting commented Sep 19, 2023

all the fuzz in #48994 is really only about node:fs only supporting URL instances. and /paths (no fileUrl string path)
that is what they are bothered about.
if we did not have node:fs, or if the filesystem was so inherity bad that it allowed folks to create files and directory with bad names, then nobody would have created #48994 and nobody would complain about it. so imo i think there is nothing wrong with it returning a string. perhaps maybe, just maybe we could improve node:fs to work with fileUrl strings instead somehow?

Personally, I would consider switching away from using node:fs if Deno, Bun, and Node.js could reach a consensus on implementing the WHATWG/fs and File System Access API. This would promote more cross-compatibility in code, eliminate the complexity of having to deal with paths, URLs, or URL strings, and ensure consistent functionality across different environments, including the browser, without relying on additional dependencies. While it might not be as developer-friendly, it would provide a consistent and dependency-free solution.

@GeoffreyBooth
Copy link
Member

Personally, I would consider switching away from using node:fs if Deno, Bun, and Node.js could reach a consensus on implementing the WHATWG/fs and File System Access API.

Sure, do you want to open an issue on https://github.com/wintercg/proposal-common-minimum-api/ to propose something? Or to just start a discussion.

nodejs-github-bot pushed a commit that referenced this issue Sep 20, 2023
PR-URL: #49698
Fixes: #49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@jimmywarting
Copy link

Sure, do you want to open an issue on https://github.com/wintercg/proposal-common-minimum-api/ to propose something? Or to just start a discussion.

Already did: wintercg/proposal-minimum-common-api#5

@lgarron
Copy link
Author

lgarron commented Sep 22, 2023

Looking good on nightly!

> ./node /tmp/test.mjs
21.0.0-nightly202309229718a9465c
file:///private/tmp/rel

ruyadorno pushed a commit that referenced this issue Sep 28, 2023
PR-URL: #49698
Fixes: #49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@lgarron
Copy link
Author

lgarron commented Oct 3, 2023

Looking good on nightly!

Also looking good inv20.8.0!

> echo "console.log(process.versions.node); console.log(import.meta.resolve('./rel'));" > /tmp/test.mjs
> node /tmp/test.mjs
20.8.0
file:///private/tmp/rel

lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node` 20.6.

This was previously available:

- returning a `Promise`,
- behind a flag, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- synchronous,
- unflagged without a parameter, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the unflagged version with flagged parameter, this change only documents the unflagged version.

Note that there was a bug that sometimes returned a `URL` instead of a `string` in v20.6.0, but this was fixed in v20.8.0 (nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node` 20.6.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
lgarron added a commit to lgarron/DefinitelyTyped that referenced this issue Oct 12, 2023
…node`.

This was previously available:

- behind a flag,
- returning a `Promise`, and
- with an optional second `parent` parameter.

As of `node` v20.6.0 (https://nodejs.org/en/blog/release/v20.6.0), it is now:

- unflagged (with a single param),
- synchronous, and
- with the `parent` parameter only being supported behind the `--experimental-import-meta-resolve` flag.

To minimize the potential of confusion that would come from mixing the
unflagged version with flagged parameter, this change only documents the
unflagged version. This also matches the API implemented by all
browsers, avoiding the potential misunderstandings about the second
parameter in codebased with mixed frontend and backend code.

Note that there was a bug that sometimes returned a `URL` instead of a
`string` in v20.6.0, but this was fixed in v20.8.0
(nodejs/node#49695).
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#49698
Fixes: nodejs#49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
targos pushed a commit to targos/node that referenced this issue Nov 11, 2023
PR-URL: nodejs#49698
Fixes: nodejs#49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
targos pushed a commit that referenced this issue Nov 23, 2023
PR-URL: #49698
Backport-PR-URL: #50669
Fixes: #49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
PR-URL: nodejs/node#49698
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
PR-URL: nodejs/node#49698
Backport-PR-URL: nodejs/node#50669
Fixes: nodejs/node#49695
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants