Skip to content

Conversation

ltk
Copy link

@ltk ltk commented Aug 4, 2016

This is just a conceptual proposal, not a final implementation.

What if 'stores' were called 'domains', and were conceptually more important?

'Store' is kind of a misnomer strictly speaking at the moment. Instead I suggest that we think instead about 'Domains', which can be thought of as representing all state management logic for a given business/application domain.

A domain looks just like an existing store, however conceptually it should include domain actions, and the entire domain object is made available via the microcosm instance.

const counterDomain = {
  actions: { // Portions of the domain could just as easily be included from other files if file length is a concern.
    updateCount: (delta) => delta
  },

  getInitialState() {
    return {
      count: 0
    }
  },

  register() {
    return {
      [counterDomain.actions.updateCount]: this.updateCount
    }
  },

  updateCount(state, delta) {
    return { ...state, count: state.count + delta }
  }
}

export default counterDomain

Using domains:

import counter from 'domains/counter'

// ...

app.addDomain('counter', counter)
// ...

// Domain organization allows, but does not force access of domain related
// systems via the main Microcosm instance:
app.push(app.domains.get('counter').actions.updateCount, 42)

src/microcosm.js Outdated

for (const domain of this.domains) {
const [key, config] = domain
if (key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we warn if there are duplicate domains?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even throw?

Copy link
Author

Choose a reason for hiding this comment

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

A warning feels appropriate. Maybe there is a legitimate use case for overwriting a domain?

Copy link
Author

Choose a reason for hiding this comment

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

duplicate/overwrites were allowed before with the metaStore (by calling addStore with no key argument, once could replace the default metaStore). Is that something we want to continue to allow?

@nhunzaker
Copy link
Contributor

nhunzaker commented Aug 5, 2016

I don't want to let this get too cold. Generally, big 👍 for a new name. Store is a relic of adhering to Flux nomenclature, which was important at the time, but less not important now.

Still thinking through the last part, with the domain key assignment, etc. I want to organize my thoughts a bit more (though still happy to hash it out in slack)

@ltk
Copy link
Author

ltk commented Aug 8, 2016

@nhunzaker curious what you think about this map-based approach I just pushed up


this.stores = this.stores.concat([[ key, config ]])
if (process.env.NODE_ENV !== 'production') {
if (key && this.domains.has(key)) {
Copy link
Author

Choose a reason for hiding this comment

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

added this in for warning about duplicates, but only if a key is present

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 156ae8f on ltk/domains into ff57ef2 on master.

@nhunzaker
Copy link
Contributor

@ltk Hesitant to require a Map polyfill. Are we getting anything else beyond has for this? Could we just use this.domains.hasOwnProperty(key)?

@nhunzaker
Copy link
Contributor

Insertion order... right. Very... very important.

@nhunzaker
Copy link
Contributor

@ltk Do you mind if I reconcile the conflicts here and remove the Map stuff? I'd like to move forward with renaming Store to Domain.

@ltk
Copy link
Author

ltk commented Sep 20, 2016

dont mind at all.

@nhunzaker
Copy link
Contributor

Invalidated in favor of #146

@nhunzaker nhunzaker closed this Sep 21, 2016
@nhunzaker nhunzaker deleted the ltk/domains branch October 12, 2016 14:21
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.

3 participants