Skip to content

[v18.x backport] lib: reset RegExp statics before running user code #44247

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[v18.x backport] lib: reset RegExp statics before running user code #44247

wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 16, 2022

Fixes: #43740

PR-URL: #43741
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Aug 16, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 16, 2022

It looks like the lint failure was introduced by 5cb5c65.

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Aug 16, 2022
This was referenced Aug 16, 2022
Merged
Closed
@danielleadams
Copy link
Contributor

@aduh95 I had to rest v18.x-staging, sorry but this will need a rebase

@aduh95
lib: reset RegExp statics before running user code
aa76869 
Fixes: nodejs#43740

PR-URL: nodejs#43741
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@aduh95 aduh95 force-pushed the backport-regexp-statics-great-reset branch from 48d92db to aa76869 Compare August 23, 2022 16:58
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Aug 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2022
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Aug 24, 2022
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 6, 2022
Open
17 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 7, 2022
Open
15 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 8, 2022
Open
12 tasks
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Sep 9, 2022
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@RafaelGSS
Copy link
Member

@aduh95 Is it ready to land? I'm about to including it to the v18 proposal

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 23, 2022

It is ready to land, thanks for taking care of this :)

@RafaelGSS
Copy link
Member

It is ready to land, thanks for taking care of this :)

Can you amend the Backport-PR-URL metadata to your commit message?

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 23, 2022

It is ready to land, thanks for taking care of this :)

Can you amend the Backport-PR-URL metadata to your commit message?

Sure I can, but I find it surprising as of all the backport PRs I've opened, I've never touched the commit message, and it was taken care of upon landing – also if I push another commit, that means we'd probably need another CI run, which I'd rather avoid. Are you sure that's not something that ncu takes care of?

@RafaelGSS
Copy link
Member

@nodejs/releasers Do ncu handle it by default? I'm just asking because following the https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md#how-to-submit-a-backport-pull-request step 9.v says:

Amend the commit message and include a Backport-PR-URL: metadata and re-push the change to your fork.

I think I can include that metadata while merging using git node land X, but I'm not quite sure if that's the correct workflow.

@RafaelGSS
Copy link
Member

Landed in 687ffcc

@RafaelGSS RafaelGSS closed this Sep 23, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 23, 2022
@aduh95 @RafaelGSS
lib: reset RegExp statics before running user code
687ffcc 
Fixes: #43740

Backport-PR-URL: #43741
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@RafaelGSS
Copy link
Member

@aduh95 just realized that I can do it manually using ncu. Sorry for the inconvenience.

@richardlau
Copy link
Member

@nodejs/releasers Do ncu handle it by default? I'm just asking because following the https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md#how-to-submit-a-backport-pull-request step 9.v says:

Amend the commit message and include a Backport-PR-URL: metadata and re-push the change to your fork.

I think I can include that metadata while merging using git node land X, but I'm not quite sure if that's the correct workflow.

Use --backport with git node land (nodejs/node-core-utils#383).

@RafaelGSS RafaelGSS mentioned this pull request Sep 23, 2022
Closed
@aduh95 aduh95 deleted the backport-regexp-statics-great-reset branch September 24, 2022 12:30
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@aduh95 @RafaelGSS
lib: reset RegExp statics before running user code
d8d34ae 
Fixes: #43740

Backport-PR-URL: #43741
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aduh95 @nodejs-github-bot @danielleadams @RafaelGSS @richardlau