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

#244 - add a createLevelDatabase to allow customizing the Level used by BlockstoreLevel #249

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

dcrousso
Copy link
Contributor

@dcrousso dcrousso commented Mar 7, 2023

this will also be passed down when creating a Dwn, MessageStoreLevel, and DataStoreLevel

Copy link
Contributor

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested some minor changes but @mistermoe and @shamilovtim probably are best people to actually know if the abstraction is sufficient for them to instantiate a different level implementation.

src/dwn.ts Outdated Show resolved Hide resolved
src/store/create-store.ts Outdated Show resolved Hide resolved
tests/store/message-store.spec.ts Outdated Show resolved Hide resolved
@dcrousso dcrousso force-pushed the drousso/createStore branch 2 times, most recently from 70bb6e5 to 03fcdaa Compare March 7, 2023 20:25
@dcrousso dcrousso changed the title #244 - add a createStore to allow customizing the Level used by BlockstoreLevel #244 - add a createDatabase to allow customizing the Level used by BlockstoreLevel Mar 7, 2023
@shamilovtim
Copy link
Contributor

I am going to try my best to review but one difficulty I'm going to have is that I can't actually pull this due to the (unrelated to PR) presence of search-index

@dcrousso dcrousso force-pushed the drousso/createStore branch 2 times, most recently from d38fc19 to f652e56 Compare March 8, 2023 02:48
@dcrousso dcrousso requested a review from thehenrytsai March 8, 2023 02:50
@dcrousso dcrousso force-pushed the drousso/createStore branch from f652e56 to e50a995 Compare March 8, 2023 02:50
@dcrousso dcrousso changed the title #244 - add a createDatabase to allow customizing the Level used by BlockstoreLevel #244 - add a createLevelDatabase to allow customizing the Level used by BlockstoreLevel Mar 8, 2023
package.json Outdated Show resolved Hide resolved
@mistermoe
Copy link
Contributor

tried it out using memory-level, works! awesome.

import type { LevelDatabase, LevelDatabaseOptions } from '../src/store/create-level';

import { MemoryLevel } from 'memory-level';
import { MessageStoreLevel } from '../src/store/message-store-level.js';
import { DataStream, DidKeyResolver, Dwn, RecordsQuery, RecordsWrite } from '../src/index.js';


const messageStore = new MessageStoreLevel({
  createLevelDatabase<K, V>(_, options?: LevelDatabaseOptions<K, V>): LevelDatabase<K, V> {
    return new MemoryLevel(options);
  },
});

(async (): Promise<any> => {
  const { did, keyPair } = await DidKeyResolver.generate();
  const data = new TextEncoder().encode('data1');

  const record = await RecordsWrite.create({
    schema                      : 'test',
    data                        : data,
    dataFormat                  : 'application/json',
    authorizationSignatureInput : {
      privateJwk      : keyPair.privateJwk,
      protectedHeader : { alg: 'EdDSA', kid: DidKeyResolver.getKeyId(did) }
    }
  });

  console.log(record.toJSON());

  const dwn = await Dwn.create({ messageStore });

  let result = await dwn.processMessage(did, record.toJSON(), DataStream.fromBytes(data));
  console.log(result);

  const query = await RecordsQuery.create({
    filter: {
      schema: 'test'
    },
    authorizationSignatureInput: {
      privateJwk      : keyPair.privateJwk,
      protectedHeader : { alg: 'EdDSA', kid: DidKeyResolver.getKeyId(did) }
    }
  });

  result = await dwn.processMessage(did, query.toJSON());
  console.log(JSON.stringify(result, null, 2));
})();

mistermoe
mistermoe previously approved these changes Mar 8, 2023
…sed by `BlockstoreLevel`

this will also be passed down when creating a `MessageStoreLevel` and `DataStoreLevel`
@dcrousso dcrousso dismissed thehenrytsai’s stale review March 8, 2023 15:19

i made the requested changes

@dcrousso dcrousso merged commit f23c825 into main Mar 8, 2023
@dcrousso dcrousso deleted the drousso/createStore branch March 8, 2023 15:20
@shamilovtim
Copy link
Contributor

I was digging through here seeing if this fits the need for various level implementations and hit a blocker. Maybe I'm missing something but we didn't remove browser level or node level. You can see this when you take a look at the level repo it's directly importing:
Screenshot 2023-03-09 at 12 00 26 PM

It imports directly from level rather than using the require() API as a fallback in an if statement for an alternative implementation of Level. It then calls new Level(...).

Changes needed

  • Provide a way to accept an alternative version of new Level()
  • Change the import of level behind an if statement using require() if someone provides a different Level

dcrousso added a commit that referenced this pull request Mar 9, 2023
i.e. if a developer provides their own `createLevelDatabase` then we will use that instead of `'level'`

in order to support this for both node and browser, we need to switch to using dynamic `import` (instead of `require`, as that's not defined for node when `"type": "module"`)

most of this commit is changing sync logic to `async` for that purpose
dcrousso added a commit that referenced this pull request Mar 9, 2023
i.e. if a developer provides their own `createLevelDatabase` then we will use that instead of `'level'`

in order to support this for both node and browser, we need to switch to using dynamic `import` (instead of `require`, as that's not defined for node when `"type": "module"`)

most of this commit is changing sync logic to `async` for that purpose (though it also allows for more flexibility when providing a custom `createLevelDatabase`)
dcrousso added a commit that referenced this pull request Mar 10, 2023
i.e. if a developer provides their own `createLevelDatabase` then we will use that instead of `'level'`

in order to support this for both node and browser, we need to switch to using dynamic `import` (instead of `require`, as that's not defined for node when `"type": "module"`)

most of this commit is changing sync logic to `async` for that purpose (though it also allows for more flexibility when providing a custom `createLevelDatabase`)
dcrousso added a commit that referenced this pull request Mar 10, 2023
i.e. if a developer provides their own `createLevelDatabase` then we will use that instead of `'level'`

in order to support this for both node and browser, we need to switch to using dynamic `import` (instead of `require`, as that's not defined for node when `"type": "module"`)

most of this commit is changing sync logic to `async` for that purpose (though it also allows for more flexibility when providing a custom `createLevelDatabase`)
thehenrytsai added a commit that referenced this pull request Mar 10, 2023
* main:
  #249- only import `'level'` when it's actually used (#254)
  #245 - updated multiformats, @ipld/dag-cbor, and interface-blockstore dependencies (#255)
  #244 - add a `tenant` parameter to each `MessageStore` method (#251)
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.

4 participants