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

Refine "require" / "import" conditions constraints #35311

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Sep 23, 2020

This PR follows up from the discussion in nodejs/modules#556 in relaxing the constraint that "require" and "import" should be exhaustive and permitting other tools to decide to match neither of these conditions effectively.

The important property of mutual exclusivity remains though.

I also took the opportunity to clarify that "import" applies for any top-level resolve or load operation in the ES module loader and that "require" is able to resolve non-CommonJS formats, to try and make the fundamental definitions of these conditions clearer for tools etc.

It's a fine line between clarify and understandability here, another reason to work towards separating "guide" information from "api" information into two halves of this section ideally over time.

Feedback welcome.

//cc @nodejs/modules-active-members @lukastaegert @sokra.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 23, 2020
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
guybedford added a commit that referenced this pull request Sep 26, 2020
PR-URL: #35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in 226b186.

@guybedford guybedford closed this Sep 26, 2020
@guybedford guybedford deleted the doc-conditions-constraint branch September 26, 2020 03:49
@Trott
Copy link
Member

Trott commented Sep 26, 2020

This landed without the required second approval. Just noting it as a polite FYI. We have a largely manual process so these things are inevitable from time to time. ¯\(ツ)/¯ One more data point that switching to the commit-queue (after it has been sufficiently real-world tested) as the primary (only?) means to land things will help us enforce a consistent process.

@guybedford
Copy link
Contributor Author

guybedford commented Sep 26, 2020

I had it in my head that the 7 day limit had been removed in a previous PR to the guidelines. Or was the waiting period only removed for PRs with two approvals? That was my misunderstanding of that change then, apologies.

@guybedford
Copy link
Contributor Author

@Trott I've created a revert PR in #35357. Happy to fast track that and wait the full time here before landing.

@Trott
Copy link
Member

Trott commented Sep 26, 2020

I had it in my head that the 7 day limit had been removed in a previous PR to the guidelines. Or was the waiting period only removed for PRs with two approvals? That was my misunderstanding of that change then, apologies.

No worries. The fact that you and 100+ other people are expected to keep the various rules straight is a bit of a process bug in my opinion. These things are simply going to happen from time to time. (Until we get a robust commit-queue that everyone is using!)

@guybedford
Copy link
Contributor Author

Well at least we have vigilant contributors to spot these things, so thanks for letting me know about it. There's no replacing a good human eye on these things :)

@Trott
Copy link
Member

Trott commented Sep 26, 2020

@Trott I've created a revert PR in #35357. Happy to fast track that and wait the full time here before landing.

I won't stop if others feel strongly about enforcing the process here, but I'd prefer we don't revert. We have enough trouble getting stuff landed. This is a narrow doc change. If someone objects to it, they can open the revert PR or better yet open a PR with further revisions that can hopefully get consensus.

@gireeshpunathil
Copy link
Member

agree no need to revert.

MylesBorins pushed a commit that referenced this pull request Sep 29, 2020
PR-URL: #35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 29, 2020
aduh95 pushed a commit to aduh95/node that referenced this pull request Oct 23, 2020
PR-URL: nodejs#35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Backport-PR-URL: #35757
PR-URL: #35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Backport-PR-URL: #35757
PR-URL: #35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35311
Reviewed-By: Jan Krems <jan.krems@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants