Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to avoid yak shave with redux_store #470

Merged
merged 1 commit into from
Jul 11, 2016
Merged

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Jul 11, 2016

We had a major yak shave when figuring out why setting up the store from
a content_for block caused majore issues.

The following doc changes and enhanced error messages will save others
from this pain.


This change is Reviewable

@justin808
Copy link
Member Author

@robwise @jbhatab Please review.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling dc60515 on fix-doc-on-shared-store into 87d2fc3 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling dc60515 on fix-doc-on-shared-store into 87d2fc3 on master.

@justin808
Copy link
Member Author

@jbhatab, @thewoolleyman @martyphee Any opinions?

We had a major yak shave when figuring out why setting up the store from
a content_for block caused majore issues.

The following doc changes and enhanced error messages will save others
from this pain.

Added a few checks for sane values when calling registerStore.
@justin808 justin808 force-pushed the fix-doc-on-shared-store branch from dc60515 to a7f600a Compare July 11, 2016 06:16
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.305% when pulling a7f600a on fix-doc-on-shared-store into 87d2fc3 on master.

@thewoolleyman
Copy link
Contributor

@jbhatab, @thewoolleyman @martyphee

Glanced at it, more docs and better messages seem good, but...

...FYI, I'm out of the loop and not doing much with React on Rails directly (although we are definitely still using it on Tracker, and plan on using it more to move towards a more mono-repo architecture for our now-separate-repo node apps, the patterns and conventions it establishes are great for that).

On my personal projects, I've backed up and am focusing on learning Elm as an alternative to Redux+React (actually learning the fundamentals of Haskell first, so I'm strong in the FP fundamentals before diving into Elm).

Not to disparage React-on-Rails, as I said, the "conventions" of structure, webpack configs, etc, are great and very much in the spirit of rails. I'm more backing up and seeing the flaws of JS/ES6 as a language, because I've seen the value of strong typing in Elm, which makes many errors like this impossible (e.g. an undefined parameter, which I believe is exactly the bug at the root of this issue). Tracker as a team I believe is also coming to this general position on Elm.

I'd love to see "react-on-rails" perhaps evolve into something that gives all the mentioned benefits of convention and "just works" webpack configs, but while allowing the use of Elm as an alternative to React+Redux. Once I learn more (which is slow going for me) I'll perhaps try to help with this.

Anyway, just a heads up on why I've gone mostly dark, sorry to put it on this random issue, but you mentioned me so wanted to be transparent. Glad to post this in a more general place if you think it's of wider interest.

Thanks,
-- Chad

@jbhatab
Copy link
Member

jbhatab commented Jul 11, 2016

node_package/src/StoreRegistry.js, line 10 [r2] (raw file):

  /**
   * Register a store generator, a function that takes props and returns a store.
   * @param storeGenerators { name1: storeGenerator1, name2: storeGenerator2 }

what's this store generator thing about?


Comments from Reviewable

@jbhatab
Copy link
Member

jbhatab commented Jul 11, 2016

node_package/src/StoreRegistry.js, line 10 [r2] (raw file):

Previously, jbhatab (Blaine Hatab) wrote…

what's this store generator thing about?

Just not sure why you broke it out to 2 exactly?

Comments from Reviewable

@jbhatab
Copy link
Member

jbhatab commented Jul 11, 2016

Reviewed 1 of 3 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


node_package/src/StoreRegistry.js, line 10 [r2] (raw file):

Previously, jbhatab (Blaine Hatab) wrote…

Just not sure why you broke it out to 2 exactly?

just clarifying the docs.

Comments from Reviewable

@justin808 justin808 merged commit 4a3933d into master Jul 11, 2016
@justin808 justin808 deleted the fix-doc-on-shared-store branch July 11, 2016 20:43
@martyphee
Copy link
Contributor

LGTM. We're not using redux anywhere and probably won't so I haven't seen anything like this.

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.

5 participants