Review requested:

• @nodejs/tsc

Code LGTM but could you please try to remove the merge commit and rebase instead? Our tooling runs into problems with merge commits.

Code LGTM but could you please try to remove the merge commit and rebase instead? Our tooling runs into problems with merge commits.

@RaisinTen Done! Thanks for the review.

@RaisinTen ci failed. I have no clue what caused it to fail. could you help to take a look?

It's a timeout on ubi81_sharedlibs_openssl111fips_x64, will rerun CI.

@RaisinTen jenkins ci timeout again. could you trigger a rerun?

The pending status in the GitHub UI is misleading because the Jenkins CI is actually fully green now.

The pending status in the GitHub UI is misleading because the Jenkins CI is actually fully green now.

great. then lets wait this pr merged.

We can get this merged in 3 days or sooner if we get another approval. :)

CI: https://ci.nodejs.org/job/node-test-pull-request/44121/

Landed in b4398df

Code LGTM but could you please try to remove the merge commit and rebase instead? Our tooling runs into problems with merge commits.

@RaisinTen are multiple commits in one pr supported by node tooling? for example, I make an initial commit and then fix some code review problems on several additional commits.

Now I always keep one commit in pr and force push to reflect code review requested change. I don't know which one is a better practice in node land. I used to always add a new commit when some change need to be added to the pr.

@F3n67u yes, our tooling supports multiple commits.

If the commits that come after the first one are just fixup commits, we can land the PR with the commit-queue-squash label, in which case the bot that lands PRs would squash all the commits into the first one for you. The PR however, would still have separate commits. So it's fine if you don't squash the commits yourself and force push.

And for PRs where you would like those to land with several commits, we could signal that to the bot by applying the commit-queue-rebase label.

So my suggestion would be to avoid force-pushing your commits into a single change because it's easier for reviewers to track the changes that were made after their last reviews. :)

However, you should consider rebasing and force-pushing if it's required to make sure that your PR works even after updating the base branch with the latest changes.

And if it's the case that the commits that follow your first one are essentially a revert and a fresh rewrite of your PR, that's when you should consider rebasing and removing the initial changes because those might be distracting and are not really needed in your PR now.

@F3n67u yes, our tooling supports multiple commits.

If the commits that come after the first one are just fixup commits, we can land the PR with the commit-queue-squash label, in which case the bot that lands PRs would squash all the commits into the first one for you. The PR however, would still have separate commits. So it's fine if you don't squash the commits yourself and force push.

And for PRs where you would like those to land with several commits, we could signal that to the bot by applying the commit-queue-rebase label.

So my suggestion would be to avoid force-pushing your commits into a single change because it's easier for reviewers to track the changes that were made after their last reviews. :)

However, you should consider rebasing and force-pushing if it's required to make sure that your PR works even after updating the base branch with the latest changes.

And if it's the case that the commits that follow your first one are essentially a revert and a fresh rewrite of your PR, that's when you should consider rebasing and removing the initial changes because those might be distracting and are not really needed in your PR now.

Got it. thanks for the info.