All tests still fail. Is this ready to merge despite?

Seems like @vapier's http-server was deleted, thus the pipeline fails.

@sgbeal, sorry to bother you! Any ideas on this pipeline failing? Has SAHPool's init changed?

haha crazy that someone used my vapier/http-server fork. I deleted it once my PRs were (finally) merged back into the canonical http-party/http-server. they sadly haven't made a new release, but if you're simply looking for cors support, you should be able to pin to that project now to get the functionality you need.

This part is curious:

in Worker (browser)
Error: Worker error: Uncaught Error: Expecting vfs=opfs|opfs-wl URL argument for this worker
 ❯ worker.onerror src/__tests__/sqlite3-sahpool-vfs.browser.test.js:20:14

SAHPool does not load sqlite3-opfs-async-proxy.js, and that's where that error comes from. If any test is trying to explicitly load that file then it will definitely fail as of 3.53. That file must now distinguish between the "opfs" and "opfs-wl" VFSes so now requires a URI flag. That file is only intended to be loaded from the code which inits the VFSes.

BTW: there's a new mechanism which can be used to force the library to not load "opfs" or "opfs-wl":

https://sqlite.org/wasm/doc/trunk/cookbook.md#disable-vfs

That will keep it from loading sqlite3-opfs-async-proxy.js.

Here's the relevant test code:

This worker is invoked here:

I haven't changed the tests at all in this PR, so it appears that the way to use SAHPool has changed now. This test is also based on real-life usage. If you inspect the test, you'll see that we don't try to manually load sqlite3-opfs-async-proxy.js.

This appears to be a breaking change from the last version we published, so we'll either need to add a migration guide, or add a patch upstream so that existing way still works.

@sgbeal, please advise on how to approach this one.

@sgbeal, please advise on how to approach this one.

The error is being triggered not from SAHPool, but from the external file sqlite3-opfs-async-proxy.js, which itself is being loaded by the part which attempts to initialize the "opfs" and "opfs-wl" VFSes. It is normal and expected for an async error message to be triggered for the case it's complaining about, but it won't (or should not) affect the library except to disable those two specific VFSes.

i'm unable to reproduce this with the upstream code so am at a loss as to what is trying to load the async proxy without the proper URI parameters. That worker is loaded from precisely one place upstream and always includes the required arguments (which are new in 3.53 but this file has always been an internal detail, not part of the public API, so clients aren't loading it manually). The relevant part is:

        const options = opfsUtil.options;
        let proxyUri = options.proxyUri +(
          (options.proxyUri.indexOf('?')<0) ? '?' : '&'
        )+'vfs='+vfsName;
        //sqlite3.config.error("proxyUri",options.proxyUri, (new Error()));
        const W = opfsVfs.worker =
//#if target:es6-bundler-friendly
              (()=>{
                /* _Sigh_... */
                switch(vfsName){
                  case 'opfs':
                    return new Worker(new URL("sqlite3-opfs-async-proxy.js?vfs=opfs", import.meta.url));
                  case 'opfs-wl':
                    return new Worker(new URL("sqlite3-opfs-async-proxy.js?vfs=opfs-wl", import.meta.url));
                }
              })();
//#elif target:es6-module
        new Worker(new URL(proxyUri, import.meta.url));
//#else
        new Worker(proxyUri);
//#/if

My assumption is that the bundler-friendly mechanism is to blame, but i've no way to confirm that in the upstream tree. Both the vanilla and non-bundler ESM modes are tested in several apps upstream and do not reproduce this.

i unfortunately don't currently have any ideas about how to resolve this.

After some struggle, I have figured it out.

The root issue is that vite strips search params from new Worker calls and replaces them with its own internal search params. This results in a following URL: sqlite3-opfs-async-proxy.js?worker_file&type=classic, instead of the expected sqlite3-opfs-async-proxy.js?vfs=opfs.

I've tried a bunch of different options to fix this on our side, but I haven't found a way, since the code that invokes the workers comes from upstream and is therefore out of our control.

The following fix also fixes just vite behavior, I'm not sure about other bundlers.

Import worker like this in the beginning of the file:

// sqlite3-bundler-friendly.js
import opfsAsyncProxy from './sqlite3-opfs-async-proxy.js?worker&url'; // <- Note the worker&url params

Then, in the switch statement that determines how to call opfs-async-proxy:

// sqlite3-bundler-friendly.js
 const W = opfsVfs.worker = (()=>{
  /* _Sigh_... */
  switch(vfsName) {
    case 'opfs': {
      const url = new URL(opfsAsyncProxy, import.meta.url);
      url.searchParams.set('vfs', 'opfs');
      return new Worker(url.toString());
    }
    case 'opfs-wl': {
      const url = new URL(opfsAsyncProxy, import.meta.url);
      url.searchParams.set('vfs', 'opfs-wl');
      return new Worker(url.toString());
    }
  }
})();

@sgbeal, @tomayac, I propose we raise this issue to vite's team first. Should they be inclined to provide a workaround on their side, the problem is solved for us.

Should they not, we either have to make these changes upstream (which is not probably not happening, since now upstream would have to deal with Node.js tooling specifics), or we have to manually implement the fix on every release.

If anyone else has a better understanding on how to resolve this, I'd be eternally grateful!

I propose we raise this issue to vite's team first.

+1 to trying that route first.

i agree, this is a vite bug. i refer you to my email from March 4th asking whether URL arguments were fine for bundling tools, and the response was "yes" ;). (i see now, though, that i neglected to notify you when it was in a final working state (as you asked me to), so i will take some blame for this problem not being discovered until this late stage.)

If vite cannot fix it, the only workaround i've been able to come up with is to create two identical copies of this file and include/load both, which would be extremely unfortunate.

I refer you to my email from March 4th asking whether URL arguments were fine for bundling tools, and the response was "yes"

Yeah, this is my bad. My use case so far has been similar to the proposed fix I described above, so I assumed they have no issues supporting it the other way as well.

I've opened a PR to vite that fixes this. I'll keep you updated on the progress! 😄

Apologies, i neglected to say: if you can confirm that your proposed approach works for other bundlers, i'll be happy to make that change upstream. i like it better than the current code.

I've been experimenting a bit more and it appears we can get it working with only this minor upstream change:

switch (vfsName) {
  case 'opfs': {
    const url = new URL('sqlite3-opfs-async-proxy.js', import.meta.url);
    url.searchParams.set('vfs', 'opfs');
    return new Worker(url.toString());
  }
  case 'opfs-wl': {
    const url = new URL('sqlite3-opfs-async-proxy.js', import.meta.url);
    url.searchParams.set('vfs', 'opfs-wl');
    return new Worker(url.toString());
  }
}

This means we don't need import opfsAsyncProxy from './sqlite3-opfs-async-proxy.js?worker&url';, which is a vite-only notation.

The tests are passing with this minor modification. I'll now try to figure out if we can prepare automated tests for this for other major bundlers as well 😄

I've been experimenting a bit more and it appears we can get it working with only this minor upstream change:

Please try applying this slightly simplified version to your 3.53 release, and if it works for you the i'll integrate it into the upstream 3.53 branch and trunk:

                const url = new URL('sqlite3-opfs-async-proxy.js', import.meta.url);
                switch(vfsName){
                  case 'opfs':
                    url.searchParams.set('vfs', 'opfs');
                    break;
                  case 'opfs-wl':
                    url.searchParams.set('vfs', 'opfs-wl');
                    break;
                }
                return new Worker(url.toString());

Which can be simplified further to:

                const url = new URL('sqlite3-opfs-async-proxy.js', import.meta.url);
                url.searchParams.set('vfs', vfsName);
                return new Worker(url.toString());

but i've no idea whether bundlers will accept that.

(Unfortunately, providing a genuine patch isn't working for me because the the Emscripten-generated pieces are producing huge amounts of diff noise for unrelated parts.)

Please try applying this slightly simplified version to your 3.53 release

Will do! We can manually patch the 3.53.0 release with this on our side, the follow up versions will include your upstream fix 😄

Patch applied upstream:

• branch-3.53 https://sqlite.org/src/info/6215c9dd27
• trunk https://sqlite.org/src/info/ec877a30f5

At your convenience, please double-check that works for you.

Alrighty, tests are passing 🥳

I've added a bunch more demo apps, at this point we have 9, so I think we're covered. We cover the majority of the functionality, along with all major bundlers.

I think we're good to merge and release this 😄