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

#249- only import 'level' when it's actually used #254

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

dcrousso
Copy link
Contributor

@dcrousso dcrousso commented 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)

shamilovtim
shamilovtim previously approved these changes Mar 9, 2023
@dcrousso dcrousso force-pushed the dcrousso/avoid-importing-level-unless-actually-used branch from 335a7fa to d5a2b51 Compare March 9, 2023 22:50
shamilovtim
shamilovtim previously approved these changes Mar 9, 2023
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.

Thanks! Looks great over all, just a couple of comments.

src/store/blockstore-level.ts Outdated Show resolved Hide resolved
src/store/create-level.ts Show resolved Hide resolved
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 dcrousso force-pushed the dcrousso/avoid-importing-level-unless-actually-used branch from d5a2b51 to 974c60a Compare March 10, 2023 04:05
@mistermoe
Copy link
Contributor

@thehenrytsai can you publish an unstable version of this so @shamilovtim can consume?

@shamilovtim
Copy link
Contributor

@thehenrytsai can you publish an unstable version of this so @shamilovtim can consume?

This one can probably just get merged. I need the unstable of the other PR and this rebased into that PR first

@thehenrytsai
Copy link
Contributor

@mistermoe and @shamilovtim, approved. Letting @dcrousso have the honor of merging. Feel free to merge if you can't wait.

@dcrousso dcrousso merged commit 58295c7 into main Mar 10, 2023
@dcrousso dcrousso deleted the dcrousso/avoid-importing-level-unless-actually-used branch March 10, 2023 18:05
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