nodejs
src,lib,test: unflag --experimental-webstorage by default
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?
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.
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?
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?
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.
@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.
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.
Codecov Report
:x: Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 88.28%. Comparing base (73b5022) to head (890a794).
:warning: Report is 116 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lib/internal/webstorage.js | 80.00% | 5 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #57666 +/- ##
==========================================
+ Coverage 88.26% 88.28% +0.02%
==========================================
Files 701 701
Lines 206644 206676 +32
Branches 39737 39738 +1
==========================================
+ Hits 182386 182472 +86
+ Misses 16279 16237 -42
+ Partials 7979 7967 -12
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| lib/internal/process/pre_execution.js | 97.15% <100.00%> (-0.28%) |
:arrow_down: |
| src/node_options.cc | 77.89% <100.00%> (+0.02%) |
:arrow_up: |
| src/node_options.h | 97.89% <100.00%> (ΓΈ) |
|
| lib/internal/webstorage.js | 91.30% <80.00%> (-8.70%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Specifically we have --no-experimental-webstorage, which should either be dropped or be aliased to --no-webstorage. Wdyt?
Yes, in a PR to stabilize, I'd rename --no-experimental-webstorage to --no-webstorage for a bit before dropping it. That said, I'd also be fine with removing it completely, but I can't remember what all of the project policies are around that anymore.
Hi @danielmbrasil and maintainers! π
I was working on this same issue independently and created a duplicate PR (#58736) which I've now closed in favor of this one. I'd love to help get this merged by addressing the remaining blockers.
Looking at the current issues, I see two main blockers:
1. Experimental Warning Inconsistency
The docs mark it as "stable" but the code still emits experimental warnings. I suggest we fully stabilize since:
- @cjihrig and @mcollina both agreed to stabilize
- Web Storage API is incredibly stable in browsers
- It's been in Node.js for a while now
Proposed fix: Remove emitExperimentalWarning('Web Storage') from lib/internal/webstorage.js
2. Flag Aliases
Need to add --webstorage/--no-webstorage aliases while keeping backward compatibility.
Proposed implementation:
// In src/node_options.cc
AddOption("--webstorage",
"Web Storage API (default: true)",
&EnvironmentOptions::experimental_webstorage,
kAllowedInEnvvar,
true);
AddOption("--experimental-webstorage",
"alias for --webstorage (deprecated)",
&EnvironmentOptions::experimental_webstorage,
kAllowedInEnvvar,
true);
Would you like me to prepare specific code suggestions for these fixes? Happy to collaborate! π€
@mcollina @cjihrig - does this approach align with your vision for stabilizing webstorage?
Failed to start CI
β Commits were pushed since the last approving review: β - src: unflag --experimental-webstorage by default β - lib: return undefined for localStorage when location is invalid β - test: remove --experimental-webstorage flag from tests β - lib: use process.emitWarning instead of emitWarning β - benchmark: remove --experimental-webstorage β - lib: remove unused import β - lib: reword warning for --localstorage-file to fix no-regex-spaces lint β - test: add localStorage and sessionStorage to globalThis test β - doc: add --no-experimental-webstorage docs β - test: improve localStorage mock to fix failing test case β - doc: fix ordering in doc/api/globals.md β - test: set getter name to 'get localStorage' in mock for compatibility β - test: use Flags comment option to set a localstorage file β - test: include test for new --no-experimental-webstorage flag β - test: add --localstorage-file flag to --no-experimental-webstorage test β - test: fix linter issue β - lib: add experimental warning back in β - doc: mark localStorage as stable β - src: add --webstorage alias β - lib: remove experimental warning β - doc: add --webstorage alias to docs β - test: fix tests β - doc: fix lint error β - lib: delay localstorage warning with proxy β - test: remove --locastorage-file flag from tests β - src,lib: rename experimental_experimental option β - doc: update docs β - lib: fix linter errors β - test: remove useless change β - doc: update node-config-schema.json β - doc: fix markdown error β - doc: fix failing tests β Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/16681856504
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}