fireproof-storage / fireproof

Realtime database, runs anywhere. Install Fireproof in your front-end app or edge function, and sync data via any backend.
https://fireproof.storage
Other
219 stars 16 forks source link

build: use tsup to bundle @fireproof/core #69

Closed thedanchez closed 5 months ago

thedanchez commented 5 months ago

This PR is the follow-up to https://github.com/fireproof-storage/fireproof/pull/61 where we use tsup as the primary tool for bundling the @fireproof packages.

thedanchez commented 5 months ago

Trying to get the tests to pass but I am running into the following error when I try to run the tests on @fireproof/core:

ReferenceError: indexedDB is not defined
      at openDB (file:///Users/danchez/Developer/workspace/github/dsanchez/fireproof/node_modules/.pnpm/idb@7.1.1/node_modules/idb/build/index.js:12:21)
      at DataStore2._withDB (file:///Users/danchez/Developer/workspace/github/dsanchez/fireproof/packages/encrypted-blockstore/dist/lib/store-web.js:250:24)
      at DataStore2.save (file:///Users/danchez/Developer/workspace/github/dsanchez/fireproof/packages/encrypted-blockstore/dist/lib/store-web.js:268:23)
      at Loader._commitInternal (file:///Users/danchez/Developer/workspace/github/dsanchez/fireproof/packages/encrypted-blockstore/dist/lib/index.js:559:25)
      at async queueFn (file:///Users/danchez/Developer/workspace/github/dsanchez/fireproof/packages/encrypted-blockstore/dist/lib/index.js:393:19)

I believe I did the alias right for the test bundles but I could be missing something. Curious if you have any thoughts @jchris ?

jchris commented 5 months ago

At one point the tests ran with a polyfill for IndexedDB -- that was before there was a node build. The memory build makes sense for external module tests (like React and Solid) but so many of the core tests inspect the filesystem, etc, so they should stay as is. Let's just focus on keeping memory building in this PR, and plan to get it working in the next round.

I think the next step here is to ensure the tests are running on the node build. In the past I had a "test" build that was just the node build but with all the files as entry points. I think I did that b/c putting all those entry points in the dist is too big.

thedanchez commented 5 months ago

@jchris I think I see what's going on -- from the stack trace, the tests are referencing store-web under the hood instead of store-node and this is all happening within @fireproof/encrypted-blockstore. It starts from index.js and then gets to store-web.

I believe I need to find a way to alias that out for the test runs so it runs against store-node. That sound correct to you?

If so, the follow-up thought that I have is, from the context of running these tests from within @fireproof/core, @fireproof/encrypted-blockstore is a pre-built dependency. First time for me to be in a situation where it appears I have to re-route the internals of a dependency package from the consuming package side of the aisle 🤔

jchris commented 5 months ago

We could have encrypted blockstore offer a bunch of builds, but I want to put the complexity somewhere else, and the EB users all probably have wild builds configured, so that's why I chose dependency injection. This way the @fireproof/core package build is the one with platform polyfill stuff going on.

jchris commented 5 months ago

the behavior you are seeing might be from the hardcoded import in crdt.ts which pulls in ./eb-web -- that's the point where the fireproof alias should impact. so we could have a test build that uses an eb-test if we need to, but I think node is the right choice

thedanchez commented 5 months ago

@jchris alright, got the alias working and the tests passing with the tsup artifacts. It's looking good. Only thing I need to check are the HTML based tests. I gave them a light run through but I wasn't able to generate Todo objects.

jchris commented 5 months ago

awsome. I am merging. My partykit-update branch involves testing the html path so when I integrate that I'll be in a position to resolve any regressions here.