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

doc: add ESM code examples in url.md #38651

Closed
wants to merge 2 commits into from
Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 12, 2021

Alternative to #38645, removing use of require or adding import equivalents where it makes sense.

@fisker what do you think?

@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
@fisker
Copy link
Contributor

fisker commented May 12, 2021

Switching to import is good, but I think removing import/require is bad. Like I said, it's not clear where these methods from.

When I first saw

new URL('file:///C:/path/').pathname;    // Incorrect: /C:/path/
fileURLToPath('file:///C:/path/');       // Correct:   C:\path\ (Windows)

I really don't know how to use fileURLToPath,

  • Is it static method of URL? URL.fileURLToPath(...)
  • Is it method of URL instance? new URL(...).fileURLToPath()
  • Or the correct one import {fileURLToPath} from 'url'
  • (edit) Or even it's global ? Like URL

It doesn't make sense to remove these imports. check https://nodejs.org/api/child_process.html, every example has require('child_process') it's very clear how to use it.

@fisker
Copy link
Contributor

fisker commented May 12, 2021

worker_threads /fs / assert/dns has those imports as well, I don't think people care much it's import or require

@aduh95
Copy link
Contributor Author

aduh95 commented May 13, 2021

@fisker Fair enough, I've added the require calls back and added ESM examples. Would you like to add ESM examples to #38645?

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2021
@fisker
Copy link
Contributor

fisker commented May 13, 2021

Yes, with pleasure.

jasnell pushed a commit that referenced this pull request May 14, 2021
PR-URL: #38651
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
@jasnell
Copy link
Member

jasnell commented May 14, 2021

Landed in df744db

@jasnell jasnell closed this May 14, 2021
@aduh95 aduh95 deleted the url-esm branch May 15, 2021 08:54
targos pushed a commit that referenced this pull request May 17, 2021
PR-URL: #38651
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
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.

5 participants