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

fix: remove node buffers #44

Merged
merged 2 commits into from
Jul 29, 2020
Merged

fix: remove node buffers #44

merged 2 commits into from
Jul 29, 2020

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jul 28, 2020

Updates to the latest changes from interface-datastore and datastore-core

Depends on:

BREAKING CHANGE: only uses Uint8Arrays internally

Updates to the latest changes from interface-datastore and datastore-core

Depends on:

- [ ] ipfs/interface-datastore#43
- [ ] ipfs/js-datastore-core#27

BREAKING CHANGES: only uses Uint8Arrays internally
Copy link

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

I think it would be better to split this pull into two distinct changes:

  1. That replaces Buffer's with Uint8Arrays
  2. That swaps glob for it-glob
  3. If error swallowing is intended make it separate.

src/index.js Outdated
value: buf
}
} catch (err) {
console.info(err)
Copy link

Choose a reason for hiding this comment

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

🚨 It looks like this would start swallowing errors, which seems unrelated to Uint8Array change.
📝 Should this be console.error(error) (or console.warn) instead ?

Copy link
Member Author

@achingbrain achingbrain Jul 28, 2020

Choose a reason for hiding this comment

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

Oops, no, this shouldn't be in there at all. It's related to a new test added to interface-datastore that mutates the datastore while running a query (a value is deleted). To pass the test we don't have to take the mutation into account but we shouldn't explode either.

@achingbrain achingbrain merged commit 887b762 into master Jul 29, 2020
@achingbrain achingbrain deleted the fix/remove-node-buffers branch July 29, 2020 12:49
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.

2 participants