search

Level / memdown

In-memory abstract-leveldown store for Node.js and browsers.
MIT License
287 stars 37 forks source link

Remove global store and location option? #84

Closed vweevers closed 6 years ago

vweevers commented 7 years ago

This feature doesn't fit Node.js patterns and even in browsers nowadays, global state is a no-no. Better put, leaking into global state. There are times when it is appropriate (e.g. React with a global store) but IMO this is an application-level choice, not to be made by a module.

Without a global store, the "location" parameter becomes irrelevant. Usage then is just memdown(). If you do need global state, you can attach the database to a global yourself, e.g. window.db = db.

WDYT @ralphtheninja @juliangruber? We can do this after releasing levelup@2.

juliangruber commented 7 years ago

+10000 for removing the global state, this has weirded me out a lot too

ralphtheninja commented 7 years ago

Go for it. I hate global state :laughing:

ralphtheninja commented 7 years ago

I want to remove the Testing section in the README as well, or at least reduce it. Too much information for someone using this module.

vweevers commented 7 years ago

I want to remove the Testing section in the README as well.

What's the harm? We have 6 npm scripts here so I think this section is a nice guide for new contributors.

vweevers commented 7 years ago

Ah, I missed your edit. Hold on, lemme open a new issue for this.

ralphtheninja commented 7 years ago

Yeah, I changed my mind. I realized removing completely was way too harsh.

vweevers commented 7 years ago

Removing location depends on Level/abstract-leveldown#63.

We can move the location !== undefined check from abstract-leveldown to leveldown and others. So we don't yet change the API (like Level/abstract-leveldown#65), we merely relax it.

Edit: that's a bit weird though. Option 2: we move the entire property from abstract-leveldown to leveldown. AFAICT abstract-leveldown no longer depends on it. But the test suite, level and levelup do..

So option 3, to start: have memdown pass a dummy location to abstract-leveldown.

vweevers commented 6 years ago

Done in v2 branch.