-
Notifications
You must be signed in to change notification settings - Fork 384
React Native Tutorial #346
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
Conversation
Love that you figured out the Jest stuff! I feel like that's a much better solution for component testing than Enzyme Reviewed 114 of 114 files at r1. mobile/ReactNativeTutorial/.babelrc, line 2 at r1 (raw file):
indent two mobile/ReactNativeTutorial/.babelrc, line 3 at r1 (raw file):
missing eof line return mobile/ReactNativeTutorial/.eslintignore, line 1 at r1 (raw file):
this is already ignored mobile/ReactNativeTutorial/.eslintignore, line 4 at r1 (raw file):
Is it necessary to ignore mobile/ReactNativeTutorial/.eslintrc.yml, line 3 at r1 (raw file):
@justin808 I would recommend also using: - plugin:lodash-fp/recommended
- plugin:flowtype/recommended mobile/ReactNativeTutorial/.eslintrc.yml, line 27 at r1 (raw file):
these last 3 shouldn't be necessary? mobile/ReactNativeTutorial/.eslintrc.yml, line 29 at r1 (raw file):
why this one? mobile/ReactNativeTutorial/.eslintrc.yml, line 30 at r1 (raw file):
I don't think we should disable this mobile/ReactNativeTutorial/.eslintrc.yml, line 31 at r1 (raw file):
I don't think we should disable this mobile/ReactNativeTutorial/.eslintrc.yml, line 32 at r1 (raw file):
this is already on mobile/ReactNativeTutorial/.eslintrc.yml, line 33 at r1 (raw file):
this is already on mobile/ReactNativeTutorial/.eslintrc.yml, line 34 at r1 (raw file):
this should be fixed now so we can remove mobile/ReactNativeTutorial/.eslintrc.yml, line 39 at r1 (raw file):
This shouldn't be necessary since you're using the mobile/ReactNativeTutorial/.eslintrc.yml, line 51 at r1 (raw file):
I think for these flow ones we should just go with mobile/ReactNativeTutorial/.flowconfig, line 67 at r1 (raw file):
might as well bump this since this implementation is greenfield mobile/ReactNativeTutorial/.watchmanconfig, line 1 at r1 (raw file):
missing eof line return mobile/ReactNativeTutorial/app/api/index.js, line 36 at r1 (raw file):
If there aren't edge cases, perhaps we should do this at the end of mobile/ReactNativeTutorial/app/api/index.js, line 37 at r1 (raw file):
I would put all schema definitions in one, separate file, and then import them here. Otherwise, you're going to have duped schema definitions all over the app. Right now it's not that bad because mobile/ReactNativeTutorial/app/bundles/comments/components/Index/Footer/Footer.js, line 16 at r1 (raw file):
Here I would just drop the fat arrow and pass the function without invoking it. This prevents extra functions from being created every time you re-render the footer, which is okay if you really need to do it, but you don't here because you aren't passing any args mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/List.js, line 17 at r1 (raw file):
This type of thing should be in the selector to prevent unnecessary re-computations of the sort when other unrelated props cause this component to re-render mobile/ReactNativeTutorial/app/bundles/comments/hocs/withIndexProps.js, line 30 at r1 (raw file):
Why not keep this selector in the selectors folder? mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 5 at r1 (raw file):
I don't think we should be putting actions inside of reducer files. Actions are not meant to have a 1-to-1 relationship with reducers like this (it defeats a lot of the usefulness of Redux). See pitzcarraldo/reduxible#8 mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 9 at r1 (raw file):
I wouldn't keep app state next to entities state as a general rule. I would put into two separate reducers mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 17 at r1 (raw file):
I think the convention is mobile/ReactNativeTutorial/app/setup/loggerMiddleware.js, line 3 at r1 (raw file):
Are you sure you don't want to just use redux-logger? It can handle Immutable if you do it like this: import createLogger from 'redux-logger';
import _ from 'lodash/fp';
const stateTransformer = _.mapValues(store => (store.toJS ? store.toJS() : store));
const loggerMiddleware = createLogger({ stateTransformer }); mobile/ReactNativeTutorial/app/setup/store.js, line 12 at r1 (raw file): const middlewares = __DEV__ ? [effectsMiddleware, loggerMiddleware] : [effectsMiddleware];
export default createStore(appReducer, initialState, applyMiddleware(...middlewares)); Also, you usually want your logging to go last because most people don't want to log promises or thunks, although I noticed you do that with your custom logger:
mobile/ReactNativeTutorial/app/setup/Router/Router.js, line 9 at r1 (raw file):
this const doesn't seem necessary, I would just inline it mobile/ReactNativeTutorial/app/utils/redux.js, line 4 at r1 (raw file):
Somewhat abstract lib utils like this can sometimes really do with a good comment, especially if this is to be used as a tutorial Comments from Reviewable |
Reviewed 108 of 114 files at r1, 11 of 11 files at r2. mobile/ReactNativeTutorial/.eslintrc.yml, line 3 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Agree with Rob! mobile/ReactNativeTutorial/.eslintrc.yml, line 30 at r1 (raw file):
mobile/ReactNativeTutorial/.eslintrc.yml, line 31 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
agreee mobile/ReactNativeTutorial/.eslintrc.yml, line 51 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
agree mobile/ReactNativeTutorial/app/api/index.js, line 36 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
This is only one level deep. I guess it's OK for a specialized case. mobile/ReactNativeTutorial/app/api/index.js, line 37 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Might be worth a try. We need to balance out simplicity vs. realism. Comments from Reviewable |
Review status: all files reviewed at latest revision, 29 unresolved discussions, some commit checks broke. mobile/ReactNativeTutorial/app/bundles/comments/components/Index/Footer/Footer.js, line 16 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
nice one mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/List.js, line 13 at r2 (raw file):
why compose and not flowRight? Yes, an alias, but now you have to know all 60 of these: https://github.com/lodash/lodash/wiki/FP-Guide#aliasesJ mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 5 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
I think this is a redux ducks pattern here mobile/ReactNativeTutorial/app/utils/redux.js, line 4 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
I agree with Rob. Yes, this file gets a new id. Comments from Reviewable |
Review status: all files reviewed at latest revision, 29 unresolved discussions, some commit checks broke. mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/List.js, line 13 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
most FP languages call this compose (as well as ramda as well as Redux's version), so even though it's listed as an alias in lodash/fp, it's actually the name you'll more commonly see, for what that's worth mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 5 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Yeah, I should say that there are people who do it this way, we (Alexey, Alex, and I) just had a talk about this and how we should be tolerant of other people's styles. If you do it this way, then you would tend to dispatch multiple actions in your thunk if you needed to change multiple reducers, making your actions more like setters. I and Abramov and Alex have some qualms with that, but at the end of the day both work. It's this point that really sets apart redux stories from redux ducks. Comments from Reviewable |
Reviewed 108 of 114 files at r1, 11 of 11 files at r2. mobile/ReactNativeTutorial/android/gradlew, line 164 at r2 (raw file):
airy! ✨ mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/ListStyle.js, line 5 at r2 (raw file):
Nothing here, is this intentional? mobile/ReactNativeTutorial/app/reducers/commentFormReducer.js, line 32 at r2 (raw file):
Why this file is not typed? mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 5 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Right, I prefer pattern mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 9 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Agreed on this separation in big apps. I treat mobile/ReactNativeTutorial/app/selectors/commentsPropsSelector.js, line 8 at r2 (raw file):
mobile/ReactNativeTutorial/app/setup/loggerMiddleware.js, line 3 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
BTW I noticed that it has issue w/ reducers, which state is not an object: https://cl.ly/iIPo (it treats them as an empty objects). mobile/ReactNativeTutorial/ios/ReactNativeTutorial.xcodeproj/project.pbxproj, line 7 at r2 (raw file):
Let's use spaces! 🤓 😅 Comments from Reviewable |
@alexfedoseev They use it in the standard libs for RN, probably that's because mobile is optimized for fonts rather than svgs I've rarely seen any svgs there. Review status: 99 of 119 files reviewed at latest revision, 35 unresolved discussions, some commit checks failed. mobile/ReactNativeTutorial/.babelrc, line 2 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.babelrc, line 3 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintignore, line 1 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintignore, line 4 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 3 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 27 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Removed React*, fetch is necessary as it's a global function in RN mobile/ReactNativeTutorial/.eslintrc.yml, line 29 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
I abandoned here the notation of $$Map for immutable and use just Map, for that I need to ignore this. mobile/ReactNativeTutorial/.eslintrc.yml, line 30 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 31 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 32 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 33 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 34 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 39 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/.eslintrc.yml, line 51 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/.flowconfig, line 67 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
I guess I'll leave it, as this is a standard React Native template, coming from the generator mobile/ReactNativeTutorial/.watchmanconfig, line 1 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/app/api/index.js, line 37 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/app/bundles/comments/components/Index/Footer/Footer.js, line 16 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/List.js, line 17 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
@robwise That's a nice one, I actually moved the sorting code to selectors long time ago, but forgot to delete this code lol ) mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/List.js, line 13 at r2 (raw file): Previously, robwise (Rob Wise) wrote…
Yeah, precisely for the reason Rob stated above. mobile/ReactNativeTutorial/app/bundles/comments/hocs/withIndexProps.js, line 30 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
I try to keep selectors uniform in terms of using only immutable. That way you can simply reuse any selector for new selector not keeping in mind whether it is JS or immutable. Once you leave selectors realm and try to connect component you convert Immutable to JS mobile/ReactNativeTutorial/app/reducers/commentFormReducer.js, line 32 at r2 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
Basically because nothing to typecheck here, all states are immutable maps, all actions are hashmaps, and each reducer either returns initialState or merges smth mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 5 at r1 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
Well in Spylight I found this approach really messy. That's because we got heavy reuse of components and reducers and the latter grew really big and clumsy. Here I use two types of actions ones are simple redux actions close to redux state structure, others are thunks close to UI structure, built on top of basic actions. I also think that this approach is in line with what we're used to in MVC approach, where redux state is model and UI are controllers - you don't put the code inside model that reacts to different controllers, you do it the other way round - controllers use model's code. mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 9 at r1 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
I agree on storing view state inside separate reducer, but here I use meta as the state of the entities state, not the part of UI. I didn't fully understand how do you propose to structure this, smth like this?
@robwise @alexfedoseev mobile/ReactNativeTutorial/app/reducers/commentsStoreReducer.js, line 17 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/app/selectors/commentsPropsSelector.js, line 8 at r2 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
Done. mobile/ReactNativeTutorial/app/setup/loggerMiddleware.js, line 3 at r1 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
I'm doing some advanced logging here and DevTools is just for web. mobile/ReactNativeTutorial/app/setup/store.js, line 12 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/app/setup/Router/Router.js, line 9 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
Done. mobile/ReactNativeTutorial/app/utils/redux.js, line 4 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/ios/ReactNativeTutorial.xcodeproj/project.pbxproj, line 7 at r2 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
Nah - tabs!!!! )))) mobile/ReactNativeTutorial/app/bundles/comments/components/Index/List/ListStyle.js, line 5 at r2 (raw file): Previously, alexfedoseev (Alex Fedoseev) wrote…
Usually I left this empty intentional, because this is very frequent way of styling, but I guess I'll drop it in the tutorial Comments from Reviewable |
013c6e0
to
f7612dd
Compare
f7612dd
to
2297fb9
Compare
I left a few comments. @robwise @alexfedoseev @alleycat-at-git Nicer that we can use basic redux-thunk. Should the mocks go into a separate repo? Reviewed 32 of 34 files at r3. mobile/ReactNativeTutorial/Readme.md, line 11 at r3 (raw file):
I recommend nvm and installing latest stable. mobile/ReactNativeTutorial/Readme.md, line 31 at r3 (raw file):
remove words "in essence" mobile/ReactNativeTutorial/mocks/mockCall.js, line 24 at r3 (raw file):
A little bit of documentation here could go a long way. Comments from Reviewable |
Review status: 120 of 122 files reviewed at latest revision, 37 unresolved discussions. mobile/ReactNativeTutorial/Readme.md, line 11 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/Readme.md, line 31 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. mobile/ReactNativeTutorial/mocks/mockCall.js, line 24 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. Comments from Reviewable |
Reviewed 2 of 34 files at r3, 2 of 7 files at r4. mobile/ReactNativeTutorial/docs/Introduction.md, line 24 at r4 (raw file):
React native is a pure UI rendering framework. To add business logic to the app, we use the Redux mobile/ReactNativeTutorial/docs/Introduction.md, line 28 at r4 (raw file):
in the following way mobile/ReactNativeTutorial/docs/Introduction.md, line 58 at r4 (raw file):
we should just merge this Comments from Reviewable |
* added react native * android sdk update * basic structure * added basic router * better logging * some styling and navigation * added api calls * api posting * added list view styling * sorting * added spinner * pull to refresh * button styling * allowed react rails for http calls * eslint * colors * flow * android support * refactored hocs into containers * added component tests * sagas first tests * replaced sagas with thunks * replaced thunks with effects * added reducers tests * added hocs tests * created common bundle * added selectors tests * flow * moved dispatch out of try * migrated to local npm * update for compatibility with thunk * external lib redux-thunk-effects * updated name, icon, bundle id, launch screen * removed outdated code * ios release * android app icon + https * android release * iOS build 2 * eslint additions * fixes for review * returned thunk * added readme.md * review comments * first docs * images fix * redux doc * Selectors docs
This change is