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

[v14.x] doc: drop import.meta.resolve parent arg URL type #38585

Closed

Conversation

kevinoid
Copy link
Contributor

@kevinoid kevinoid commented May 7, 2021

I could not find any version of Node.js which accepts a parent argument of type URL. For example, on Node.js 16.1.0, index.mjs with content:

import.meta.resolve('./index.mjs', new URL(import.meta.url));

produces:

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

TypeError [ERR_INVALID_ARG_TYPE]: The "parentURL" argument must be of type string. Received an instance of URL
    at new NodeError (node:internal/errors:363:5)
    at validateString (node:internal/validators:119:11)
    at Loader.resolve (node:internal/modules/esm/loader:87:7)
    at Object.resolve (node:internal/modules/esm/translators:122:26)
    at file:///path/to/index.mjs:1:13
    at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
    at async Loader.import (node:internal/modules/esm/loader:178:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Therefore, update the docs to drop the URL type for this argument.

Thanks for considering,
Kevin

cc: @guybedford, since you are the author of these docs and an expert in all things import.

@github-actions github-actions bot added the doc Issues and PRs related to the documentations. label May 7, 2021
@aduh95
Copy link
Contributor

aduh95 commented May 7, 2021

Thanks for sending this PR! Alternatively I've opened #38587 to add support for URL objects.

@kevinoid
Copy link
Contributor Author

kevinoid commented May 7, 2021

Sounds great. That'd be handy for me. Thanks @aduh95!

jasnell pushed a commit that referenced this pull request May 10, 2021
PR-URL: #38587
Refs: #38585
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
moander pushed a commit to moander/node that referenced this pull request May 11, 2021
PR-URL: nodejs#38587
Refs: nodejs#38585
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
moander pushed a commit to moander/node that referenced this pull request May 11, 2021
PR-URL: nodejs#38587
Refs: nodejs#38585
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented May 15, 2021

4ebb88f have landed on master and should be included in the next v16.x release and backported to v14.x soon after in the next semver-minor release, scheduled in August. Would you like to repurpose this PR to fix the v12.x documentation? after checking, this section is not in the v12.x docs, there nothing to fix there after all 😅

You could repurpose this PR to fix the v14.x documentation, which could land in the next v14.x patch release schedule in June. Of course it's totally OK if you're not interested in working on this, someone else can pick it up – or it could simply wait for the August release.

@kevinoid kevinoid force-pushed the doc-import-meta-resolve-type branch from 2614203 to d8987a6 Compare May 16, 2021 00:24
@kevinoid kevinoid changed the base branch from master to v14.x-staging May 16, 2021 00:24
@kevinoid
Copy link
Contributor Author

Thanks @aduh95, great work!

I've retargeted for v14.x-staging and added your doc version comment from #38587, since that was a nice addition. (Note: I used 12.16.0 instead of 12.16.2 after confirming it was present. If that will cause problems, I can use 12.16.2 as well.)

Good catch about the v12.x docs. Would it make sense for me to create a v12.x backport PR for import.meta.resolve docs? Or was it left out of the docs intentionally?

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

I used 12.16.0 instead of 12.16.2 after confirming it was present. If that will cause problems, I can use 12.16.2 as well.

I copied the info from here:

node/doc/api/cli.md

Lines 225 to 232 in 0996eb7

### `--experimental-import-meta-resolve`
<!-- YAML
added:
- v13.9.0
- v12.16.2
-->
Enable experimental `import.meta.resolve()` support.

When looking at the changelogs, it is also listed in the v12.16.2 release:

* [[`05091d48e3`](https://github.com/nodejs/node/commit/05091d48e3)] - **esm**: import.meta.resolve with nodejs: builtins (Guy Bedford) [#31032](https://github.com/nodejs/node/pull/31032)

I find it very surprising you are seeing it available before v12.16.2, are you sure it's not added by some kind of extension?
If you're able to confirm it present in v12.16.0, it'd be nice to open a PR to fix the docs in master.

Good catch about the v12.x docs. Would it make sense for me to create a v12.x backport PR for import.meta.resolve docs? Or was it left out of the docs intentionally?

This section of the docs was part of a larger refactor of the docs in #36046, it hasn't been backported to v12.x yet because so far no one took the time to do it I believe. If you want to open a backport PR that'd be awesome but it's also OK to leave it as is.

@aduh95 aduh95 changed the title doc: drop import.meta.resolve parent arg URL type [v14.x] doc: drop import.meta.resolve parent arg URL type May 16, 2021
@aduh95 aduh95 added the v14.x label May 16, 2021
Support for parent argument of type `URL` was added in nodejs#38587, which has
not been backported to previous releases yet.

Update docs for v14.x to remove the URL type for this argument.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@kevinoid kevinoid force-pushed the doc-import-meta-resolve-type branch from d8987a6 to 30f1408 Compare May 16, 2021 13:09
@kevinoid
Copy link
Contributor Author

Thanks for investigating the ChangeLog details. I don't know what I was thinking last night. 12.16.2 looks correct to me (both from the commit log and re-testing each 12.16 patch release with NVM). Sorry for the hassle! PR updated.

Thanks for the explanation about the v12.x docs! Good call. I'll probably leave that as is.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 16, 2021
targos pushed a commit that referenced this pull request May 17, 2021
PR-URL: #38587
Refs: #38585
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
Support for parent argument of type `URL` was added in #38587, which has
not been backported to previous releases yet.

Update docs for v14.x to remove the URL type for this argument.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #38585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
Copy link
Member

targos commented May 30, 2021

Landed in 43f4668

@targos targos closed this May 30, 2021
@kevinoid
Copy link
Contributor Author

Great! Thanks @targos and @aduh95!

targos pushed a commit that referenced this pull request Jun 5, 2021
Support for parent argument of type `URL` was added in #38587, which has
not been backported to previous releases yet.

Update docs for v14.x to remove the URL type for this argument.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #38585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Support for parent argument of type `URL` was added in #38587, which has
not been backported to previous releases yet.

Update docs for v14.x to remove the URL type for this argument.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #38585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
Support for parent argument of type `URL` was added in #38587, which has
not been backported to previous releases yet.

Update docs for v14.x to remove the URL type for this argument.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

PR-URL: #38585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
PR-URL: #38587
Refs: #38585
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants