lib: string indexOf to use primordials for http2/compat.js by rchougule · Pull Request #36679 · nodejs/node · GitHub

StringPrototypeIncludes primordial in place of <string>.indexOf for truthy validation in if expression.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Can you please squash the fixup commit. Else LGTM 👍🏽

Note to whoever lands this: the first word of the commit message should be an imperative verb, I suggest something like http2: refactor to use primordials instead of <string>.indexOf.

Can you please squash the fixup commit. Else LGTM 👍🏽

@yashLadha This is usually done by the collaborator who lands the PR (git node land 36679 --fixupAll). They are also in charge of amending the commit message if there are typos or if it doesn't follow the commit guidelines.

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

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

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

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/856/ (queued, will 404 until it starts running)

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/856/ (queued, will 404 until it starts running)

Why does it say NA? or am I looking at the wrong place? 😅

Why does it say NA? or am I looking at the wrong place? 😅

See #36746

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

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

@aduh95 can you help me understand the further steps here? Just trying to understand what's failing here. Thanks!

can you help me understand the further steps here?

We need to have a green CI in order to land this; the failure on the CI are almost certainly unrelated to the changes introduced by this PR, I've just kicked off another one, hopefully it should be good now.

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

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

It seems the same checks failed 😓 ... @aduh95

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

PR-URL: #36679 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Pooja D P <Pooja.D.P@ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Landed in 3af175f

@Trott It seems that multiple PRs are failing for the 3 jobs probably a flakiness issue.

@Trott It seems that multiple PRs are failing for the 3 jobs probably a flakiness issue.

Yeah, it can be frustrating when CI is broken. If a test is known to be faulty, then a good thing to do is open a fast-track PR to add a line to the .status file telling CI to ignore the test results for that test That's what eventually happened in this case, and that got us back to yellow (and we'll be back to green once nodejs/build#2521 lands).

LGTM @Trott

Subsequent benchmark of a revert on CI didn't show anything of interest, so this is good.

PR-URL: #36679 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Pooja D P <Pooja.D.P@ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Dec 29, 2020

aduh95 approved these changes Dec 29, 2020

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

Lxxyx approved these changes Dec 30, 2020

View reviewed changes

himself65 approved these changes Dec 30, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 30, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020

PoojaDurgad approved these changes Dec 31, 2020

View reviewed changes

aduh95 mentioned this pull request Dec 31, 2020

Cannot start CI jobs nodejs/build#2506

Closed

rchougule force-pushed the primordials_lib_http2 branch from 4bb6fab to 9d927f3 Compare December 31, 2020 11:56

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020

jasnell reviewed Dec 31, 2020

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

jasnell approved these changes Dec 31, 2020

View reviewed changes

http2: refactor to use primordials instead of <string>.indexOf

12c78aa

rchougule force-pushed the primordials_lib_http2 branch from fcc389b to 12c78aa Compare December 31, 2020 18:19

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021

rchougule mentioned this pull request Jan 1, 2021

zlib: refactor to use primordial instead of <string>.startsWith #36718

Closed

2 tasks

Trott approved these changes Jan 2, 2021

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

This comment has been minimized.

Sign in to view

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2021

yashLadha closed this Jan 9, 2021

This comment has been minimized.

Sign in to view

Trott mentioned this pull request Jan 11, 2021

benchmark: fix http2 benchmarks #36871

Merged

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos added the backport-blocked-v14.x label May 25, 2021

lib: string indexOf to use primordials for http2/compat.js by rchougule · Pull Request #36679 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: string indexOf to use primordials for http2/compat.js - #36679

lib: string indexOf to use primordials for http2/compat.js#36679rchougule wants to merge 1 commit intonodejs:masternodejs/node:masterfrom rchougule:primordials_lib_http2rchougule/node:primordials_lib_http2Copy head branch name to clipboard

Conversation

rchougule commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

yashLadha commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 31, 2020

Uh oh!

Uh oh!

yashLadha commented Jan 1, 2021

Uh oh!

Trott commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchougule commented Jan 3, 2021

Uh oh!

aduh95 commented Jan 3, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 5, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

rchougule commented Jan 6, 2021

Uh oh!

aduh95 commented Jan 6, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

rchougule commented Jan 7, 2021

Uh oh!

nodejs-github-bot commented Jan 8, 2021

Uh oh!

yashLadha commented Jan 9, 2021

Uh oh!

yashLadha commented Jan 10, 2021

Uh oh!

Trott commented Jan 10, 2021

Uh oh!

This comment has been minimized.

yashLadha commented Jan 11, 2021

Uh oh!

Trott commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lib: string indexOf to use primordials for http2/compat.js#36679
rchougule wants to merge 1 commit into
nodejs:masterfrom
rchougule:primordials_lib_http2

rchougule commented Dec 29, 2020 •
edited
Loading

Trott commented Jan 2, 2021 •
edited
Loading