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

docs: update code examples for node:url module #38645

Merged
merged 1 commit into from
May 19, 2021
Merged

Conversation

fisker
Copy link
Contributor

@fisker fisker commented May 12, 2021

There are many things called url in this page including url module,
URL instances, etc.

The original example was not clear where these methods come from.

@github-actions github-actions bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels May 12, 2021
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2021

Thanks for sending this, I'm not sure we want to do that: it raises questions such as "Why go for require instead of import?" or "Should we include "use script"; in the code snippets?". The current code snippets are less rigorous as they couldn't run by themselves, but maybe that's OK for an example to leave out the boilerplate.

@fisker
Copy link
Contributor Author

fisker commented May 12, 2021

Why go for require instead of import?

Because all other codes use require in this file. (this can answer to other similar questions too)

@fisker
Copy link
Contributor Author

fisker commented May 19, 2021

// @aduh95 Added esm examples as request in #38651 (comment)

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

  • I feel it's more natural to use the named import for fileURLToPath instead of url.fileURLToPath, but I reckon it's a personal taste and I don't feel strongly about it.
  • I'm not sure we want to update the legacy section with ESM examples, this section of the docs is there for maintainers of legacy code base which would be written in CJS. wdyt?

doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
doc/api/url.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: nodejs#38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented May 19, 2021

Landed in 42cceff, thanks for the contribution 🎉

@aduh95 aduh95 merged commit 42cceff into nodejs:master May 19, 2021
@fisker fisker deleted the url-docs branch May 19, 2021 17:20
danielleadams pushed a commit that referenced this pull request May 31, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: #38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request May 31, 2021
richardlau pushed a commit that referenced this pull request Jul 16, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: #38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: #38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: #38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Jul 20, 2021
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
There are many things called `url` in this page including `url` module,
`URL` instances, etc.

The original example was not clear where these methods come from.

PR-URL: nodejs#38645
Reviewed-By: Antoine du Hamel <duhamelantoine1995@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. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants