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 miss line for cjs #49608

Closed
wants to merge 1 commit into from
Closed

Conversation

fishel-feng
Copy link

doc: add miss line for cjs

doc: add miss line for cjs
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Sep 11, 2023
@fishel-feng fishel-feng mentioned this pull request Sep 11, 2023
@ShogunPanda
Copy link
Contributor

I think the message should be doc: add missing line for cjs example, but I'm not sure if this is a blocker.

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

Thanks for the PR. process is available on the global object, it's not necessary to require it. It's not a missing line, it was omitted on purpose.

@ShogunPanda
Copy link
Contributor

In that case shall we remove it from ESM? (and eventually look for other places in our docs?

@fishel-feng
Copy link
Author

Thanks for the PR. process is available on the global object, it's not necessary to require it. It's not a missing line, it was omitted on purpose.

@aduh95 It's not equivalent with the ESM demo, If we do omit it, it should be removed.

@fishel-feng
Copy link
Author

fishel-feng commented Sep 11, 2023

In that case shall we remove it from ESM? (and eventually look for other places in our docs?

@ShogunPanda I just make it same to the ESM demo. And most of the demo use manually import for process. There isn't a norm right now. It is recommended that all documents be checked if necessary.

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

Thanks for the PR. process is available on the global object, it's not necessary to require it. It's not a missing line, it was omitted on purpose.

@aduh95 It's not equivalent with the ESM demo, If we do omit it, it should be removed.

Who said that CJS and ESM examples needed to be line-by-line equivalent?

In that case shall we remove it from ESM? (and eventually look for other places in our docs?

No, for ESM we made the choice to always import Node.js-specific globals.

@ShogunPanda
Copy link
Contributor

Thanks for the PR. process is available on the global object, it's not necessary to require it. It's not a missing line, it was omitted on purpose.

@aduh95 It's not equivalent with the ESM demo, If we do omit it, it should be removed.

Who said that CJS and ESM examples needed to be line-by-line equivalent?

No one, it was my assumption.

In that case shall we remove it from ESM? (and eventually look for other places in our docs?

No, for ESM we made the choice to always import Node.js-specific globals.

I see, didn't know about it. We don't need to do anything then.
If you get the chance (and will, and time), can you tell me (or link me a issue/discussion/whatever) on reason behind this decision?

@fishel-feng
Copy link
Author

fishel-feng commented Sep 11, 2023

No, for ESM we made the choice to always import Node.js-specific globals.

In other examples this norm is not followed. No one said anything was necessary, but I think it is necessary to follow a unified processing method. You can just turn off the PR. It's not my project, and it has nothing to do with me whether it's good or bad.

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

In other examples this norm is not followed.

Do you have specific examples where this is the case? It seems we should fix those.

@aduh95
Copy link
Contributor

aduh95 commented Sep 11, 2023

If you get the chance (and will, and time), can you tell me (or link me a issue/discussion/whatever) on reason behind this decision?

It was done in #39043.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants