Skip to content

Conversation

@privatenumber
Copy link
Contributor

Addresses #51033 (comment)

PR waiting on #51033

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Dec 5, 2023
@GeoffreyBooth
Copy link
Member

Why is this a new PR? Can you please just update the branch for #51033 with this code?

Then if you can refactor --experimental-loader into --import / register it can land.

@privatenumber
Copy link
Contributor Author

I prefer to organize my PRs by the problem they address

To me, including this in #51033 doesn't make sense because the code changes in this PR are not necessary to fix the problem it address (#50839)

@GeoffreyBooth
Copy link
Member

I understand that’s how you prefer to organize your PRs, but in this project we squash all commits for every PR, and we want each PR to be as contained as possible because we need to backport the squashed commits onto multiple release lines (21.x, 20.x, 18.x, etc.). In other words, we try to minimize the number of commits on main because each extra commit there is more work for the release team to prepare each release. Responding to code review notes is something that we expect people to do on the branch associated with the opened PR, not in new PRs.

@privatenumber
Copy link
Contributor Author

Okay, in that case, I'll address your comments exclusively to the changes I made.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Dec 5, 2023

People often just push new commits to respond to each code review note. It doesn’t matter how many commits are on a branch because they all end up getting squashed anyway; but until the PR lands, if you ever need the history of how the PR looked at an earlier point, you still have it.

Thanks for working with us!

@GeoffreyBooth GeoffreyBooth added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Dec 5, 2023
@privatenumber
Copy link
Contributor Author

It was less about the commits (as I often squash/amend my commits in a PR anyway). More about the scope of the change and how easy it is for a 3rd person to understand relevant code changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate Issues and PRs that are duplicates of other issues or PRs. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants