Skip to content

src: refactor webstorage implementation - #53876

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:refactor-webstorage
Jul 18, 2024
Merged

src: refactor webstorage implementation#53876
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:refactor-webstorage

Conversation

@anonrig

@anonriganonrig commented Jul 16, 2024

Copy link
Copy Markdown
Member

I couldn't find a good way to split them into multiple pull-requests.

@anonrig
anonrig requested a review from cjihrigJuly 16, 2024 20:37
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 16, 2024
@anonrig
anonrigforce-pushed the refactor-webstorage branch from 8af37ce to bb81c4eCompareJuly 16, 2024 20:49

@cjihrigcjihrig left a comment

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.

I'm not sure if the tests will pass with these changes, but LGTM if they do pass.

@anonriganonrig added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 16, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@cjihrig

Copy link
Copy Markdown
Contributor

As I suspected, the tests appear to be failing. The inputs to these APIs do not need to be strings, but must be able to be converted to strings.

@anonrig

Copy link
Copy Markdown
MemberAuthor

As I suspected, the tests appear to be failing. The inputs to these APIs do not need to be strings, but must be able to be converted to strings.

TIL. So, apparently null and undefined values are converted into string with the current approach. Reverting those changes now. Thanks for the review @cjihrig

@anonrig
anonrigforce-pushed the refactor-webstorage branch from bb81c4e to e53f1adCompareJuly 16, 2024 23:11
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 17, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 05b7bf0 into nodejs:mainJul 18, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 05b7bf0

targos pushed a commit that referenced this pull request Jul 28, 2024
PR-URL: #53876
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jul 30, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@anonrig@nodejs-github-bot@cjihrig@jasnell@targos