Skip to content
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

Add support for compound PK batch ops in test suite #16

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

poltak
Copy link
Member

@poltak poltak commented Jul 16, 2019

I'm not entirely sure if I'm going about this the correct way.
I saw the empty should support batch operations with compound primary keys test and thought maybe that's what I should expand on (and which I did).
Then I realized we'd need some new backend feature support flag for compound keys, so I added in compoundPrimaryKeys.

Though I noticed running the test suite from the root storex workspace directory, the should support batch operations with compound primary keys test runs for the firestore backend (which I haven't touched), so maybe I'm wrong about the need for a new feature support flag.

How far am I off the correct path here @ShishKabab ?

@poltak poltak requested a review from ShishKabab July 16, 2019 03:53
@ShishKabab
Copy link
Member

ShishKabab commented Jul 16, 2019

Yeah, that's exactly right: that should be a feature flag. You can move the Firestore test here if it's easy to do, since I think I put it in there only to cover an immediate use case in a quick and dirty way.

One thing I noticed though, is that the primary key in your new tags collection is not defined as I'd expect. Isn't it usually done using something that looks life this?

{ field: ['name', 'email'], pk: true },

Or in the top of the collection definition:

pkIndex: ['name', 'email']

Have you checked whether the compound index is really created by Dexie for example?

- also added new backend feature support flag to check support of
@poltak poltak force-pushed the feature/compound-pk-tests branch from 66f2c26 to 451bea1 Compare July 21, 2019 08:08
@poltak
Copy link
Member Author

poltak commented Jul 21, 2019

So I've had a look in the firestore repo and I don't actually see that test; I think I misunderstood that earlier in the test output (I was a bit rushed submitting this PR). As far as I can see running the test suite now, that test does not run for the firestore repo. Unless you know of the test existing in the firestore repo, let's ignore that.

The actual issue I have now is:
When I go into the TypeORMStorageBackend (in the TypeORM submodule repo of storex-workspace) and add the new compoundPrimaryKeys flag, then run the test suite again (yarn test in the root of the storex-workspace repo), nothing changes and that test still does not run under the TypeORM StorageBackend tests suite.

Now inside the TypeORM submodule repo, from what I can see, it imports and resolves the @worldbrain/storex package from node_modules/@worldbrain/storex rather than the storex submodule that exists adjacent to the TypeORM submodule (i.e., where these changes exist - they do not exist in the node_modules/ version as they should have been downloaded from NPM).

I'm feeling like I'm not using the storex-workspace repo in the way it's intended to be used, or I'm missing some step/s to ensure the submodules are aware of each other rather than trying to use the packages hosted on NPM.

One thing I noticed though, is that the primary key in your new tags collection is not defined as I'd expect. Isn't it usually done using something that looks life this?

Yes I think you're right here. I've updated the commit in this PR.

@ShishKabab
Copy link
Member

I think you're running into the issue that if you install any new package through yarn, the symlinks to the adjacent packages will get overwritten. You can confirm this by running ls -l node_modules/@worldbrain and not seeing any symlinks. Fix is to go to the storex-workspace root and running yarn bootstrap --ignore-scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants