-
Notifications
You must be signed in to change notification settings - Fork 29.3k
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
deps,lib,src: add experimental web storage #52435
Conversation
Review requested:
|
@nodejs/tsc can we add this to tomorrows meeting? |
Looking at #50169 (review), I think all of those concerns are addressed by making SQLite an internal dependency that is not exposed in any way to the user; is that the case in this PR? If so, then I think the block has been addressed, and there’s nothing to override. |
Due to the history of this PR I propose we just quickly bring it up and approve it at the TSC to make sure there is no further frustration. |
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
I’m all for adding |
This is currently the same implementation as #50169.
The specific review containing the objection is not really relevant in my opinion. However, not trying to "keep up with the Joneses" (referring to Deno, txiki.js, and potentially Bun) was mentioned, as was a sentiment opposing browser APIs.
This is indeed not ready to land yet. I'm not going to invest any more time on it though until I know the TSC supports landing it in principle. I do believe the PR is far enough along for that decision to be made fairly easily. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m in favor of landing this.
The @nodejs/tsc discussed this today and there is consensus on having web storage land. Obviously there are technical details to work through but there are no objections to this moving forward. There is also positive consensus on adding sqllite as an internal dependency in support of the implementation. Let's get this done :-) |
return; | ||
} | ||
CHECK(domexception_ctor_val->IsFunction()); | ||
Local<Function> domexception_ctor = domexception_ctor_val.As<Function>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking Nit: The code necessary to create a DOMException
should likely be moved out into a separate utility method.
I appreciate it. I'll start addressing the feedback from this PR and the old one soon. As a side note, I noticed on the TSC call that a few people had some technical questions, such as "why SQLite, specifically instead of writing to disk." I tried to answer this and other questions in #50169 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I am +1 on eventually landing this and like other experimental/new stuff I am in favor of landing this sooner rather than later and then iterating on it.
There is a lot of work to be done to make this stable but I think iterating usually is. abetter strategy than big changes and a lot of criticism.
My understanding is the TSC discussed this and approved the idea in principle so this is me +1ing (I couldn't attend due to a prod incident)
Note to self: create tickets after this lands with all the issues (e.g. range queries and others) so they can be addressed or discussed before this is stable. |
@cjihrig did you consider using:
Would any of the above provide slightly better characteristics (performance, binary size, source code size, manutenibility)? Note that this does not matter much, and we can change the storage mechanism later (before it becomes stable). |
Please see #50169 (comment) for my answer to why I chose SQLite. |
I'd like to publicly agree with Colin on SQLite here. I've also done a few projects with various options for embedded databases and have found SQLite to be an excellent choice due to its ease of integration, ultra-permissive license (public domain), omnipresence across hardware which means good support etc. localStorage + sharing readers across workers seems right up its alley. A file with a global lock would work but would lead to a lot less extensibility and would make us solve problems in the future SQLite would avoid. (If anything, we should eventully consider exposing SQLite the same way Python does) |
There might be a slight issue when adding SQLite as a dependency. It uses a non-approved license that is not OSI. We would need to seek legal+board approval to land this. @nodejs/tsc can you weight in? As an alternative, we could use libsql that is MIT license (but is a fork of sqlite). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love this! I'm not experienced enough with C++ to give that part a proper review, and I just scanned the WPT as I assume if they are passing then they are good to go.
I'll keep an eye on some of the other changes being introduced if they change.
Great work and thank you!
// max_size is 10MB. This can be made configurable in the future. | ||
" max_size INTEGER NOT NULL DEFAULT 10485760," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only apply to sessionStoage
or to localStorage
too? If yes, in the docs, you only mentioned the limit for sessionStorage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with two nits/questions
std::to_string(kCurrentSchemaVersion) + ";"; | ||
r = sqlite3_exec(db, set_user_version_sql.c_str(), 0, 0, nullptr); | ||
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a rough feeling we would need to implement a migration framework here. Is that risk mitigated somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to use the schema version for managing any required migrations.
> Stability: 1.0 - Early development. | ||
|
||
A browser-compatible implementation of [`Storage`][]. Enable this API with the | ||
[`--experimental-webstorage`][] CLI flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should note that this is backed by SQLite and all actions are synchronous and blocking as they might cause the event loop to stall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the synchronous bit - this is a spec'ed API, so I'm not sure if it makes sense to explain that type of detail. I'd also prefer not to go through the whole CI process again for minor docs things that can more easily be done in a followup PR.
Landed in 340cb68...15b3902 |
This commit introduces an experimental implementation of the Web Storage API using SQLite as the backing data store. PR-URL: #52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This is equivalent to the following upstream change: https://sqlite.org/src/info/6c103aee6f146869 Original commit message: Change constant expressions to pre-computed constants, because apparently MSVC on ARM requires that. [forum:/forumpost/4feb1685cced0a8e|Forum thread 4feb1685cced0a8e]. FossilOrigin-Name: 6c103aee6f146869a3e0c48694592f2e4c6b57ecdb4450f46e762c38b4e686f1 PR-URL: #52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This commit introduces an experimental implementation of the Web Storage API using SQLite as the backing data store. PR-URL: #52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This is equivalent to the following upstream change: https://sqlite.org/src/info/6c103aee6f146869 Original commit message: Change constant expressions to pre-computed constants, because apparently MSVC on ARM requires that. [forum:/forumpost/4feb1685cced0a8e|Forum thread 4feb1685cced0a8e]. FossilOrigin-Name: 6c103aee6f146869a3e0c48694592f2e4c6b57ecdb4450f46e762c38b4e686f1 PR-URL: #52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This commit introduces an experimental implementation of the Web Storage API using SQLite as the backing data store. PR-URL: nodejs#52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This is equivalent to the following upstream change: https://sqlite.org/src/info/6c103aee6f146869 Original commit message: Change constant expressions to pre-computed constants, because apparently MSVC on ARM requires that. [forum:/forumpost/4feb1685cced0a8e|Forum thread 4feb1685cced0a8e]. FossilOrigin-Name: 6c103aee6f146869a3e0c48694592f2e4c6b57ecdb4450f46e762c38b4e686f1 PR-URL: nodejs#52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This commit introduces an experimental implementation of the Web Storage API using SQLite as the backing data store. PR-URL: nodejs#52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
This is equivalent to the following upstream change: https://sqlite.org/src/info/6c103aee6f146869 Original commit message: Change constant expressions to pre-computed constants, because apparently MSVC on ARM requires that. [forum:/forumpost/4feb1685cced0a8e|Forum thread 4feb1685cced0a8e]. FossilOrigin-Name: 6c103aee6f146869a3e0c48694592f2e4c6b57ecdb4450f46e762c38b4e686f1 PR-URL: nodejs#52435 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Reopening of #50169, which cannot be reopened. Opening as a draft because the original PR had a block, so the TSC would need to make a decision (#50169 (comment)).
This commit introduces an experimental implementation of the Web Storage API using SQLite as the backing data store.