Skip to content
This repository has been archived by the owner on Oct 1, 2021. It is now read-only.

fix: remove node globals #6

Merged
merged 3 commits into from
Apr 14, 2020
Merged

fix: remove node globals #6

merged 3 commits into from
Apr 14, 2020

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Mar 31, 2020

This PR add new datastore for browser and removes node globals.
Also opens datastore in several places!

This PR add new datastore for browser and remove node glovals
Plus
Copy link
Member

@AuHau AuHau left a comment

Choose a reason for hiding this comment

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

LGTM.

Will the switch to datastore-idb need a migration?

@hugomrdias
Copy link
Member Author

it shouldn’t, do we have any tests for that ?

@AuHau
Copy link
Member

AuHau commented Apr 1, 2020

Nope, this tool depends on the datastore abstraction and this abstraction is not tested here.

I am not following js-ipfs development lately so not sure what this change is bringing in the bigger scope. If js-ipfs moves to this datastore as default one and it uses different storage mechanism, then there should be migration to move the data from the old one to new one 😉

@hugomrdias
Copy link
Member Author

backend is the same its just removing one abstraction layer, but still we should keep snapshots to be able to test this type of stuff

@AuHau
Copy link
Member

AuHau commented Apr 1, 2020

Hmmm I am not really sure what or how to test for this. I mean we can test the abstraction, but I don't think this is the place to do it.

Do you have a proposal what exactly to test and how?

@hugomrdias hugomrdias requested a review from AuHau April 8, 2020 15:11
@hugomrdias hugomrdias merged commit 800c27f into master Apr 14, 2020
@hugomrdias hugomrdias deleted the fix/remove-node-globals branch April 14, 2020 15:15
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.

3 participants