Review requested:

• @nodejs/gyp

Making my objection official for the reasons #49663 (comment)

This is not quite the same thing as that. With the exception of what is stored to disk, sqlite is not exposed to users at all.

Sqlite has a lot of compile-time knobs

The same could be said for a lot of Node's dependencies. This is also not a generic SQL API.

Sqlite is missing some features that would be desirable to general users (e.g. encryption)

Encryption at rest is possible for anyone that wants to take the time to encrypt the database file or individual pieces of data stored in the database.

there are lots of opinionated ways to present APIs

The API here comes from a spec.

There is also a lot of prior art. Deno ships web storage backed by SQLite. Bun exposes SQLite, and is considering adding web storage. The smaller txiki.js project ships both SQLite and web storage. Every major browser ships web storage.

Making my objection official for the reasons I described in the original thread about this:

@mscdex Those were objections to a public SQLite API made available by Node; this is a PR about exposing the Web Storage API, not SQLite. Do you have any objections to the Web Storage API being made available by Node?

Do you have any objections to the Web Storage API being made available by Node?

That was covered by the first part of my objection. Just because browsers/Deno/Bun/whatever is doing it, doesn't mean it needs to exist in node or that node needs to "keep up with the Joneses".

That was covered by the first part of my objection. Just because browsers/Deno/Bun/whatever is doing it, doesn’t mean it needs to exist in node or that node needs to “keep up with the Joneses”.

Web standard APIs are a category that we do need to support. We don’t necessarily need to implement them using SQLite, but in general we want to support as many browser APIs as possible. It’s not a competition thing, it’s a compatibility thing.

we want to support as many browser APIs as possible

Not everyone feels that way.

Not everyone feels that way.

It’s one of the project’s technical values: https://github.com/nodejs/node/blob/main/doc/contributing/technical-values.md

It’s fine if you personally disagree, but I don’t think a disagreement with a goal of the project is a valid reason to object to an individual PR.

@mscdex The technical priorities don't exactly say "we want to support as many browser APIs as possible" and as a part of the group who worked on those I don't think we meant that exactly. That said, I do think this is in line with "Web API compatibility", and if you disagree with that item I think that conversation should happen. Would love to have you in a @nodejs/next-10 session to discuss.

That said, the priorities were adopted and I agree with @GeoffreyBooth that it is not a valid reason to object to an individual PR.

The technical priorities don't exactly say "we want to support as many browser APIs as possible" and as a part of the group who worked on those I don't think we meant that exactly. That said, I do think this is in line with "Web API compatibility"

I'd be interested to hear why you think "Web API compatibility" and "we want to support as many browser APIs as possible" are not the same thing. Is there an established definition that makes the difference between "web" and "browser" absolutely clear when it comes to APIs? I don't think I've ever seen a "web" API originate from somewhere other than a browser.

I'd be interested to hear why you think "Web API compatibility" and "we want to support as many browser APIs as possible" are not the same thing.

We had quite lengthy discussions about what this means. I will have to dig to find all the notes, but the TLDR is: We would make a best effort to implement web apis which made sense in a Node.js context. And secondly that we would strive for spec compliance when it made sense, but not require it for all cases.

EDIT: finding those notes is hard, and really not useful unless you are going to watch the meeting video I think as they are not comprehensive. That said, here is a rough search you could look through.

Additionally, regarding the project's technical priorities, it also includes "operational qualities" such as "small binary size" and "small memory footprint", which seems at odds with adding an endless number of browser or "web" APIs.

seems at odds with adding an endless number of browser or "web" APIs.

We also have prioritization and a structure for those considerations. https://github.com/nodejs/next-10/blob/main/VALUES_AND_PRIORITIZATION.md#values-and-priority-level

And this IMO falls under developer experience.

And this IMO falls under developer experience.

Erm... "Web API compatibility" is listed under "Up to date technology and APIs" in the technical priorities document and the prioritization list you linked to puts that at the lowest priority ("Technology and API currency").

We discussed why we put technical and api currency on the bottom, but api's which users want is a developer experience thing. We did discuss that, but maybe that nuance was lost in the final doc.

EDIT: Sorry, hard to remember many many meetings off the top of my head. We would love to have more collaborators attend those so the context is not lost in our heads but was instead a generally agreed upon direction. Sadly that is not the case.

Second Edit: I think this is quite off topic. I think we should have this discussion in the context of the next-10 group, not this PR.

The final doc is this one: https://github.com/nodejs/node/blob/main/doc/contributing/technical-values.md. Browser compatibility is listed under Priority 1 - Developer experience.

Compatibility and interoperability with browsers and other JavaScript environments so that as much code as possible runs as is both in Node.js and in the other environments

Ah you are right @GeoffreyBooth! Thanks, I forgot that we made edits after the initial drafts in the next 10 repo.

The final doc is this one: https://github.com/nodejs/node/blob/main/doc/contributing/technical-values.md. Browser compatibility is listed under Priority 1 - Developer experience.

Both documents are the same regarding order of priorities, there are just some minor wording changes.

So again, what is the difference between "Web API compatibility" and something like "Compatibility and interoperability with browsers and other JavaScript environments so that as much code as possible runs as is both in Node.js and in the other environments". To me the latter sounds more like "let's ensure that our existing setTimeout() works the same way as it does in the browser" and "Web API compatibility" is about new features, especially considering "Supporting and exposing new technologies and standards through early adoption" is in the same category.

I get that this discussion is not directly related to the PR, but I think it is very pertinent since it is a new API. Also anecdotally, I've never heard of anyone complain about or refuse to consider using node.js for a project because it did not have a built-in storage database (of any kind).

So again, what is the difference between

What does it matter what the difference is? They’re both priorities. A committee tasked with sketching out Node’s future decided that they were both part of where we want the project to go.

I think we should respect the Next 10 team’s work and treat their documents as our guiding principles, and PRs that are in line with those goals are to be welcomed. Collaborators can always object based on specifics, like around SQLite versus in-memory store; but completely disregarding the Next 10 results is basically ignoring our own user research, and I think we should all have some humility and trust that the people tasked with doing that investigation know best what our users want.

anecdotally, I’ve never heard of anyone complain about or refuse to consider using node.js for a project because it did not have a built-in storage database (of any kind).

Sure, but the fact that Bun and Deno both offer this—and promote it—is a pretty strong indicator that there’s user demand.

You can also review our most recent survey. In https://github.com/nodejs/next-10/blob/main/surveys/2023-04/Node.js%20Survey%20open%20ended%20answers.pdf, when asking users about issues they have with Node:

• “Missing Web API’s compatibility. In Deno I can take most of the code from web and check it on backend. For instance localstorage - it’s super convenient to ‘just use it’ on backend instead of playing with some file access libraries.”
• “No proper inmemory database for quick db testing”

More generally:

• “aligning with other web standards to have isomorphic implementation for both the front-end and the back-end”
• “Full compatibility with Web API’s (like Deno)”
• “Respect more WebApi”

And lots of mentions of specific Web APIs like Websockets, Web Workers and so on.

Out of curiosity, what's the impact on the binary size?

I think we should respect the Next 10 team’s work and treat their documents as our guiding principles, and PRs that are in line with those goals are to be welcomed. Collaborators can always object based on specifics, like around SQLite versus in-memory store; but completely disregarding the Next 10 results is basically ignoring our own user research, and I think we should all have some humility and trust that the people tasked with doing that investigation know best what our users want.

Strong +1 here.

My 2cents, Node definitely should implement WebStorage APIs. It would be yet another gaming-changing API that Developers need so often, and that sadly so many libraries, frameworks need to monkey-patch or provide 3rd party solutions just to make this "trivial" functionality to work.

Also, @mscdex you've made your points, and your objection is noted. I think for the sake of productivity, and to avoid escalating this PR unnecessarily, please avoid engaging unnecessarily.

Now I wonder if more people are going to object to this feature, and if no consensus is reached, (as someone from Next-10) I'd like to raise TSC intervention (as last resort, if we go nowhere), as (it's not just my personal opinion) but as survey's and the market tell out, this is a feature that our userbase want.

Now, is the implementation of this feature going to affect Node.js negatively? (security or performance-wise, or because the initial implementation is undesirable?) If yes, then we should wait for people from the security and performance teams to chime in. (As we definitely want to weight the pros and cons of implementing this)

(I'm just trying to be objective here)

“No proper inmemory database for quick db testing”

On a personal note, I wish we supported a Key-Value store. As Deno and Bun does. Now, I'm not sure if SQLite is the best approach here. Maybe we should stick with something memory-based (runtime-based) instead of a file, such as SQLite. (I know SQLIte also supports in-memory, but SQLite in general is a hefty dependency to add... We must also consider OS-compatibility)

According to sqlite page, current version is 3.43.2

https://www.sqlite.org/releaselog/3_43_2.html

Also: I really like https://github.com/WiseLibs/better-sqlite3 by @JoshuaWise
Maybe the contributors are willing to give us some feedback regarding the performance?

Opened an issue in better-sqlite3

WiseLibs/better-sqlite3#1086

I'm -0.5 on adding localStorage for the reasons in #49663 (comment) - executive summary: it's just not a very good API.

Taking on a big dependency (check the size of the diff!) for something that's meh at best... eh, I'd rather not.

It’s one of the project’s technical values: https://github.com/nodejs/node/blob/main/doc/contributing/technical-values.md

Appeal to authority. Kind of a conversation killer, that one.

Those values are written up by the authors of that document and they make up only a fraction of the maintainer base. Other maintainers can disagree, and they should if they feel something is bad.

Just to clarify, I did not open this PR as a proposal for a generic KV store (which is why I did not originally include any references or metadata pointing to that issue). I opened this solely in the context of the web storage API which seems like something Node may be interested in providing.

@bnoordhuis :

without really discussing alternative technologies, of which there are several

Which other approaches would you like to discuss? I'm definitely open to considering other approaches (but we should probably do so in a discussion or issue thread so we don't cause too much noise here getting in the way of code review for this specific PR).

@cjihrig:

We would need to decide how we want to handle worker threads. I noted this in my original post - in the browser, web workers do not have access at all, but Node's worker threads are not exactly web workers.

For now, at least with the initial implementation of this, I'd say it would make sense for globalThis.localStorage to not be available within a worker thread at all. Once we figure out exactly how that should work relative to the main thread we can turn it on. Not having that available, of course, limits some of the utility here but I think it's the safest option while we figure that bit out.

Allowing globalThis.sessionStorage in a worker thread right away is safer but I do think every worker thread should have it's own distinct sessionStorage separate from each other and separate from the main thread. I'm more than willing to be convinced otherwise tho. Perhaps it would be possible to explicitly share these through options passed into the worker (similar to the stdio options):

const w = new Worker(..., {
  localStorage: globalThis.localStorage,
  sessionStorage: globalThis.sessionStorage,
});

But this can certainly be done later before graduating from experimental

In terms of SQLite, I do like that is a single .c file and also looking at recent vulnerabilities it seems like there have not been too many and the project documents their assessment of CVEs - https://sqlite.org/cves.html.

In terms of licence we might need to ask for an exception as Public Domain" is not on the preferred OpenJS Foundation list. - https://sqlite.org/copyright.html

Reopening as it looks like maybe this wasn't picked up by the TSC agenda because it's closed?

To be honest, I really don't like how the conversation has gone down here. We can have a civil respectful discussion about our API surface.

Please, when someone like Brian or Ben raises an objection hear them out. They are not debating in bad-faith and are smart competent core members. We don't all have to agree but let's please listen to each other. Let's remember why we use a consensus seeking model in the first place.

I feel like there is discussion to be had about whether or not this API belongs in core. I think we can have a constructive discussion weighing the pros and cons of inclusion namely:

• Are the APIs good?
• Are they a widely supported standard?
• Are they widely used?
• What is the impact on Node's size?
• What is the impact on Node's performance?
• What is the impact on Node's security?
• What is the burden of maintenance?
• What is the impact of increasing the API surface?

I feel like there is discussion to be had about whether or not this API belongs in core. I think we can have a constructive discussion weighing the pros and cons of inclusion namely:

That's a really good list of items here, thanks for coming up with those.

Please, when someone like Brian or Ben raises an objection hear them out. They are not debating in bad-faith and are smart competent core members.

I re-read a couple of times all the comments, and I don't see anyone here getting out of line or people trying to be dismissive. Yet, +1 that we should maintain a healthy conversation here. We're all smart people here all with good intents. Let's avoid using passive aggressive terms :)

I feel this is a really good conversation to ping @nodejs/web-standards group we created recently, as the members there supposedly should be people with strong knowledge about Web APIs and Web Standards.

I can answer a few of these.

• Are the APIs good?
• Are they a widely supported standard?
• Are they widely used?

localStorage, at least, is quite good and I use it all the time. It’s supported by all browsers. It’s not obvious how it makes sense in a server context, if your concept of what a Node application does is respond to incoming network requests; but Node applications can also be clients making fetch calls of their own, rendering webpages like browsers would (either for testing purposes or as part of server-side rendering) and in these contexts it makes sense for a Node application to behave similarly to browsers. Supporting localStorage and sessionStorage increases the capabilities of Node in that new abilities are unlocked (at least, without mocking the APIs via something like JSDom).

@cjihrig Why was this closed? Was there a discussion in TSC?

We ran out of time during the TSC meeting and did not have a chance to discuss this.

We did shortly discuss this on the 8 nov meeting. We didn't have sufficient participants to vote to override the objection. But the consensus for the attending members was that the objection itself was not relevant for the PR as nothing of SQLite would be exposed externally.

We did raise some concerns regarding a few points that we would recommend further discussion on. But it was quite similar to what Benjamin wrote above:

• What are the security implications?
• What are the maintainability implications? (compiling, CI etc...)
• What is the upgrade path if we for some reason decide to replace SQLite in the future?

I don't see any reason to close this. I would personally expect the objection to be overridden next time this comes up at the TSC. The concerns raised were more of a recommendation of consideration than something that necessarily needs to be addressed.

Excuse me for resurrecting this, but what happened here ?
did it get ruled out in TSC or not ?

The TSC never made a decision so I closed the PR. And since this PR had a block, nothing can happen without the TSC making a decision.

I would personally push for the TSC to make a decision if the PR was re-opened (I'm in the TSC).

This PR cannot be reopened, so I've opened a draft PR at #52435.