Skip to content

Revert "src: migrate String::Value to String::ValueView" - #55828

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
targos:revert-55458
Nov 16, 2024
Merged

Revert "src: migrate String::Value to String::ValueView"#55828
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
targos:revert-55458

Conversation

@targos

@targostargos commented Nov 12, 2024

Copy link
Copy Markdown
Member

This reverts commit 45c6a9e.

Closes: #55826

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 12, 2024
avivkeller
avivkeller reviewed Nov 12, 2024
View reviewed changes
Comment threadsrc/inspector_js_api.cc
Comment on lines -248 to +250

TwoByteValue task_name_buffer(args.GetIsolate(), args[0]);
StringView task_name_view(*task_name_buffer, task_name_buffer.length());
Local<String> task_name = args[0].As<String>();
String::Value task_name_value(args.GetIsolate(), task_name);
StringView task_name_view(*task_name_value, task_name_value.length());

@avivkelleravivkellerNov 12, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a full revert, I feel like this part can stay the same?

@targostargosNov 12, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A full revert is easier to work with. You're free to make incremental changes after it.

@aduh95aduh95Nov 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good illustration of why atomic PRs should always be preferred.

@avivkeller

avivkeller commented Nov 12, 2024

Copy link
Copy Markdown
Member

Is there a test that can be added to ensure this doesn't happen again?

@targos

targos commented Nov 12, 2024

Copy link
Copy Markdown
MemberAuthor

I wasn't able to find a repro that doesn't involve webpack.

@codecov

codecovBot commented Nov 12, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 60.71429% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (3a0968d) to head (df0b75f).
Report is 95 commits behind head on main.

Files with missing linesPatch %Lines
src/string_bytes.cc20.00%8 Missing ⚠️
src/node_buffer.cc80.00%1 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #55828 +/- ##
==========================================
- Coverage 88.41% 88.41% -0.01% 
==========================================
Files 654 654 Lines 187811 187817 +6 Branches 36134 36132 -2 ==========================================
+ Hits 166052 166056 +4 - Misses 15008 15011 +3 + Partials 6751 6750 -1
Files with missing linesCoverage Δ
src/inspector_js_api.cc86.29% <100.00%> (+0.05%)⬆️
src/node_buffer.cc70.06% <80.00%> (-0.12%)⬇️
src/string_bytes.cc68.43% <20.00%> (-0.46%)⬇️

... and 31 files with indirect coverage changes

@targostargos added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 12, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024
@nodejs-github-bot

nodejs-github-bot commented Nov 12, 2024

Copy link
Copy Markdown
Collaborator

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

@avivkeller

avivkeller commented Nov 13, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Sorry for breaking this, everyone! Thanks for the speedy fix

@avivkelleravivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. revert PRs that revert previously landed PRs. labels Nov 13, 2024
@nodejs-github-bot

nodejs-github-bot commented Nov 14, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Nov 16, 2024

Copy link
Copy Markdown
Collaborator

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

@avivkelleravivkeller mentioned this pull request Nov 16, 2024
Closed
@chenrui333chenrui333 mentioned this pull request Nov 16, 2024
Merged
6 tasks
willxj36 added a commit to willxj36/Interactive-Resume that referenced this pull request Nov 16, 2024
@willxj36
enforce node 23.1.x as 23.2.x has broken webpack builds (pending PR h…
7ed499a
…ere: nodejs/node#55828)
@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 16, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 16, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 9f2885a into nodejs:mainNov 16, 2024
@nodejs-github-bot

nodejs-github-bot commented Nov 16, 2024

Copy link
Copy Markdown
Collaborator

Landed in 9f2885a

@targos
targos deleted the revert-55458 branch November 17, 2024 06:13
RafaelGSS pushed a commit that referenced this pull request Nov 18, 2024
@targos@RafaelGSS
Revert "src: migrate String::Value to String::ValueView"
58d7a6e
This reverts commit 45c6a9e.
PR-URL: #55828Fixes: #55826
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Nov 19, 2024
Merged
@emyl3emyl3 mentioned this pull request Nov 20, 2024
Merged
@ruyadorno

ruyadorno commented Nov 27, 2024

Copy link
Copy Markdown
Member

This commit does not land cleanly on v22.x-staging and will need manual backport in case we want it in v22.x.

@ruyadornoruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024
@richardlau

richardlau commented Nov 27, 2024

Copy link
Copy Markdown
Member

This reverts #55458 which is marked https://github.com/nodejs/node/labels/dont-land-on-v22.x so should be similarly labelled.

@richardlaurichardlau added dont-land-on-v18.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 27, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 approved these changes
@avivkelleravivkelleravivkeller left review comments
@benjamingrbenjamingrbenjamingr approved these changes
@lpincalpincalpinca approved these changes
@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes
@jazellyjazellyjazelly approved these changes
@jakecastellijakecastellijakecastelli approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.revertPRs that revert previously landed PRs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

23.2 breaks a few packages that use json trough webpack (date-fns)

11 participants

@targos@avivkeller@nodejs-github-bot@ruyadorno@richardlau@benjamingr@lpinca@gireeshpunathil@aduh95@jazelly@jakecastelli