feat: add base logic for getting Topics via redux #11

kgajowy · 2018-05-27T20:28:03Z

Added:

eslint rules to jest
added thunk to have more powerful dispatching (redux-observable does not support redux v4 :(
made the state as it should be (divided into props per feature/part of app) instead of 'global')
TopicContainer tests (with actions mocking)
TopicReducer Tests
ActivityIndicator
Topic's actions specs

Tuhaj · 2018-05-27T20:33:14Z

src/reducers/topics.reducer.js

+    switch (action.type) {
+    case TOPICS_GET_REQUEST:
+        //TODO add ..state, and tests!
+        //TODO add thunk as redux-observable is out of order :(


… properly dispatch actions in correct order

Tuhaj · 2018-05-31T12:40:37Z

.eslintrc.js

@@ -3,7 +3,8 @@ module.exports = {
        'browser': true,
        'commonjs': true,
        'jasmine': true,
-        'es6': true
+        'es6': true,
+        'jest/globals': true


cool! WebStrom won't underline these globals?

yes! :) (describe/it only tho:)

Tuhaj · 2018-05-31T12:41:10Z

package.json

@@ -9,6 +9,8 @@
        "react-redux": "^5.0.7",
        "react-scripts": "1.1.4",
        "redux": "^4.0.0",
+        "redux-thunk": "^2.2.0",
+        "rxjs": "^6.2.0",


is RX needed if we have redux-thunk?

I see we don't import anything from this library, so I guess you pushed it accidentally

you are right, thanks!

I need to investigate it further, as without it lint-stage is throwing this error:
lint-staged/lint-staged#416

fixed in 7646cd7
Please note that in my case it was necessary to totally remove lint-staged from both node_modules and package.json and install it again :(

Tuhaj · 2018-05-31T12:42:11Z

src/actions/topic.actions.js

+        return TopicService.getSubmittedTopics().then(topics => {
+            dispatch(topicsFetched(topics));
+        }).catch(error => {
+            dispatch(topicsFetchError(error));


props for error handling 👍

it is not reflected anywhere in the app yet - I suppose it would be great to use some Toast/SnackBars to show them!

Tuhaj · 2018-05-31T12:42:42Z

src/components/TopBar.js

@@ -38,7 +38,7 @@ TopBar.propTypes = {
 };

 const mapStateToProps = state => ({
-    loggedUser: state.loggedUser
+    loggedUser: state.auth.loggedUser


yeah, better use this scopes

Tuhaj · 2018-05-31T12:44:06Z

src/index.js

+    compose(
+        applyMiddleware(
+            thunk),
+        window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__()


using window, super pure!

Tuhaj · 2018-05-31T12:47:22Z

src/reducers/__tests__/topics.reducer.test.js

+
+    describe('payload received', () => {
+        const payload = [TopicModel.fromBackendData({})];
+        const topicState = topicReducer({pending: true}, topicsFetched(payload));


aha! I guess it was the initial state, so maybe empty object in previous cases (no preference here, I just want to open a discussion how to make this in a best way)

Passing null would actually break the reducer state - as first argument would be null, then default assignment is not used (fun(arg=default, ...) { ...} ), therefore it would blow the whole reducer (try to change undefined to null - reducer always return null instead of initialState)
Empty object {} works the same - no initialState is used. We could just pass initialState which is already exported but I am not sure if it gives us a real value. What do you think?

yeah, good idea!

Tuhaj · 2018-05-31T12:50:23Z

src/reducers/auth.reducer.js

+    case LOGIN:
+        return Object.assign({}, state, { loggedUser: true });
+    case LOGOUT:
+        return Object.assign({}, state, { loggedUser: false });


yeh, we stay with es6. I saw recently what problems can there be with Object Rest/Spread is still a proposal! https://github.com/tc39/proposal-object-rest-spread CC: @ulaulala

Interesting, I haven't know about any bugs for that. @Tuhaj could you please elaborate more? If I understood correctly from this issue (tc39/proposal-object-rest-spread#52) it uses Object.assign under the hood.

@kgajowy No bugs, but a lot of bother in case of deploy on SOME platforms. We recently tried to deploy a node app on Azure, and guess what happened: transpilers needed. Luckly on Heroku it went without any problems.

Tuhaj · 2018-05-31T12:52:35Z

src/topic/__tests__/TopicContainer.test.js

+         * 2) while using a dive, we have actions called immediately which may not be what we want
+         *
+         * I picked 2) as using 1) would only tell us that prop is called but we would lose the assurance that
+         * Container is correctly 'connected' (i.e. has correctly mapped dispatchToProps


interesting, have you experienced some bugs with wrongly connected components before?

@Tuhaj lets say so - lack of correct mapping or lack of connect at all :)

Tuhaj · 2018-05-31T12:53:23Z

src/topic/__tests__/TopicContainer.test.js

+    });
+
+    it('should have topics array connected correctly from redux', () => {
+        expect(wrapper.instance().props.topics).toBe(initialState.topic.topics);


this still confuses me, .props().topics won't work here?

The full example is provided there http://airbnb.io/enzyme/docs/api/ShallowWrapper/props.html
I think it is worth reading but to make it short: wrapper.props() returns only those props that are directly mentioned within the construction of <Component props....) - while we have some props that are connected by the Redux! thus:

const mapStateToProps = ({ topic }) => ({ topics: topic.topics, pending: topic.pending, error: topic.error });

no prop from those will be visible in wrapper.props()!

so in fact, we actually could pass all props instead of the store there... but it would be a test that doesnt actually test if our component builds correctly in connection to the Redux

thanks for explanation!

Tuhaj · 2018-05-31T12:54:00Z

src/topic/components/__tests__/TopicsList.test.js

@@ -41,4 +46,14 @@ describe('<TopicsList>', () => {
        });
    });

+    describe('when topics are loading', () => {
+        beforeEach(() => {
+            wrapper = shallow(<TopicsList pending={true}/>);


const missing

I don't think it is necessary. let wrapper is defined at the very beginning of the test. Or you mean sosmething else than const wrapper ? (btw it wouldn't be visible in the test later or, so rather let wrapper inside describe but this is already done above)

eh, haven't noticed /shrug leave it

feat: add base logic for getting Topics via redux

066c6b6

kgajowy added enhancement work in progress labels May 27, 2018

Tuhaj reviewed May 27, 2018

View reviewed changes

kgajowy added 7 commits May 27, 2018 22:54

update: add thunk to the redux store as middleware; modify actions to…

605867b

… properly dispatch actions in correct order

update: topics.reducer tests

8ac465c

update: <TopicContainer> tests

22849a3

chore: remove unused deps

85b3c59

update: add specs for TopicContainer

4e333a8

chore: add ActivityIndicator and use it in TopicsList

80b729a

chore: add topic action creator tests

143183d

kgajowy removed the work in progress label May 31, 2018

kgajowy requested review from piecioshka, Tuhaj and ulaulala May 31, 2018 11:41

Tuhaj approved these changes May 31, 2018

View reviewed changes

chore: remove unused rxjs dependency

7646cd7

kgajowy merged commit 7f1783c into master Jun 1, 2018

kgajowy deleted the feature/topics_getter branch June 1, 2018 11:58

piecioshka assigned kgajowy Nov 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add base logic for getting Topics via redux #11

feat: add base logic for getting Topics via redux #11

kgajowy commented May 27, 2018 •

edited

Loading

Tuhaj May 27, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018 •

edited

Loading

Tuhaj May 31, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

kgajowy May 31, 2018

kgajowy May 31, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

Tuhaj May 31, 2018

Tuhaj May 31, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

Tuhaj Jun 1, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

Tuhaj Jun 1, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018 •

edited

Loading

Tuhaj Jun 1, 2018

Tuhaj May 31, 2018

kgajowy May 31, 2018

Tuhaj Jun 1, 2018

feat: add base logic for getting Topics via redux #11

feat: add base logic for getting Topics via redux #11

Conversation

kgajowy commented May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgajowy May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgajowy May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgajowy commented May 27, 2018 •

edited

Loading

kgajowy May 31, 2018 •

edited

Loading

kgajowy May 31, 2018 •

edited

Loading