Skip to content
This repository has been archived by the owner on Sep 30, 2023. It is now read-only.

feat: add the optional argument cache to the constructor #42

Merged
merged 1 commit into from
Nov 7, 2019
Merged

feat: add the optional argument cache to the constructor #42

merged 1 commit into from
Nov 7, 2019

Conversation

tmorin
Copy link
Contributor

@tmorin tmorin commented Oct 28, 2019

Hi

The pull request related to #41.

Best regards

constructor (input = {}) {
if (typeof input === 'string') {
this._store = createStore(input)
} else if (typeof input.open === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the purpose of checking input.open? Are you treating the options object as the store? Imo we should expect the store to be passed as options.store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @shamb0t,

From my understanding, the current API allows three cases:

  1. no arguments
  2. a string which must be a path
  3. a level store

So yes the first if block checks if input is a level store (case .3).
And the second one checks if input is a string, i.e. a path (case .2).

The intend is to avoid breaking changes.

However, if breaking changes are OK on your side I can review the constructor implementation accordingly.

Best regards

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

@shamb0t
Copy link
Contributor

shamb0t commented Nov 7, 2019

Apologies for the delay @tmorin 🙏 happy to merge this, thank you!

closes #41

@shamb0t shamb0t merged commit 3797fdb into orbitdb-archive:master Nov 7, 2019
@tmorin
Copy link
Contributor Author

tmorin commented Nov 7, 2019

@shamb0t no problems.

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

Successfully merging this pull request may close these issues.

None yet

2 participants