Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

src,lib,test: unflag --experimental-webstorage by default #57666

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

danielmbrasil
Copy link
Contributor

Resolves #57658

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Mar 28, 2025
@mcollina
Copy link
Member

To fix the failing test, I would change the error being throw in lib/internal/webstorage.js in a warning, and have localStorage be undefined in that case.

@cjihrig wdyt?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 28, 2025

I would be OK with that behavior. However, the global checking logic in test/common/index.js would probably need to be updated. Otherwise, the new warning will probably be printed for every test that doesn't use localStorage.

@danielmbrasil
Copy link
Contributor Author

I just pushed the suggested changes, and the tests are now passing. @cjihrig, could you please review whether the changes in test/common/index.js make sense?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 28, 2025

Without running the code, it doesn't look correct to me:

globalThis[val] === undefined

By the time we find out that the value is undefined, the warning will have already been emitted, right?

@danielmbrasil
Copy link
Contributor Author

By the time we find out that the value is undefined, the warning will have already been emitted, right?

Yeah, that's right. I'll take a look on how to avoid the warning to be emitted in those cases.

@danielmbrasil danielmbrasil force-pushed the src/unflag-experimental-webstorage branch from b925ca4 to db0cda3 Compare April 2, 2025 17:11
@danielmbrasil
Copy link
Contributor Author

@cjihrig Could you take a look now and see if this is closer to what you were expecting? I added a new check to test/common/index.js that overrides the localStorage getter with an empty object. This prevents the warning from being emitted, and the tests are passing locally.

@danielmbrasil danielmbrasil changed the title src: unflag --experimental-webstorage by default src,lib,test: unflag --experimental-webstorage by default Apr 4, 2025
@danielmbrasil
Copy link
Contributor Author

Moving this as ready for review. I think we should possibly also update doc/api/cli.md and doc/api/globals.md, but I'm not sure how to update those docs for now.

@danielmbrasil danielmbrasil marked this pull request as ready for review April 4, 2025 18:12
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (b85505d) to head (e1ec2f3).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57666      +/-   ##
==========================================
- Coverage   90.24%   90.23%   -0.02%     
==========================================
  Files         630      630              
  Lines      185198   185205       +7     
  Branches    36295    36304       +9     
==========================================
- Hits       167140   167126      -14     
- Misses      10987    10994       +7     
- Partials     7071     7085      +14     
Files with missing lines Coverage Δ
lib/internal/webstorage.js 100.00% <100.00%> (ø)
src/node_options.cc 85.21% <ø> (-0.13%) ⬇️
src/node_options.h 98.88% <100.00%> (ø)

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
Copy link
Member

mcollina commented Apr 7, 2025

A few tests needs fixing but this is good progress!

@danielmbrasil danielmbrasil force-pushed the src/unflag-experimental-webstorage branch from ce12b27 to 67c3460 Compare April 7, 2025 16:47
@danielmbrasil danielmbrasil force-pushed the src/unflag-experimental-webstorage branch from 67c3460 to 1857c7f Compare April 7, 2025 16:51
Comment on lines +319 to +321
const getLocalStorage = Object.defineProperty(() => {}, 'name', {
value: 'get localStorage',
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels a bit hacky, but I wasn’t able to find a better solution. Happy to hear any suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unflag --experimental-webstorage
4 participants