Move injectors code to external library by BenLorantfy · Pull Request #2749 · react-boilerplate/react-boilerplate

BenLorantfy · 2019-09-19T22:04:09Z

This PR moves the injector-related code to a new external library called redux-injectors. For the full discussion on how we got here, see this issue. A few guiding notes:

See this file for the changes made to the store setup: https://github.com/react-boilerplate/react-boilerplate/pull/2749/files#diff-ea94aca90fbef7f544d2a96cf1da5f8e
- Removes the store mutation in favour of using a store enhancer
- Store is now given createReducer. It was only given runSaga previously.
This is an example of how the imports would change: https://github.com/react-boilerplate/react-boilerplate/pull/2749/files#diff-b40dc3de2b66cf46597584200a5599f7R14
Removed a bunch of code:
- Removed the injectReducer, injectSaga, checkStore, and sagaInjectors files
- Removed the related tests
- Removed the same files in the templates folder
Added docs/js/redux-injectors.md. I linked to the redux-injectors docs for readers to view the full docs, but let me know if I should go into more detail in this file.
Changed a few documentation files to use redux-injectors (e.g. docs/js/redux-saga.md, docs/js/redux.md)
Removed some steps from docs/js/remove.md
Updated the generators to use redux-injectors
Updated all the templates with the same changes that were done to the source code

A few other links:

redux-injectors on npm: https://www.npmjs.com/package/redux-injectors
redux-injectors on GitHub: https://github.com/react-boilerplate/redux-injectors

julienben · 2019-09-22T11:05:46Z

😍😍😍

Quick question: Why do we need to add aliases for react deps in the webpack config?

BenLorantfy · 2019-09-22T14:05:50Z

That was just for local development and is needed when using yarn link to link redux-injectors to react-boilerplate. Planning on removing it once redux-injectors is published and before merging this.

BenLorantfy · 2019-10-24T15:09:17Z

OK, this is now ready for review whenever anyone has a chance 🙂

BenLorantfy · 2019-10-24T15:10:28Z

@julienben When you have some time, I also need help transferring redux-injectors to the reactboilerplate org as I explained in this comment: #2698 (comment)

julienben · 2019-10-25T11:44:54Z

I think the transfer is done:

Will review the code asap. Please notice that CI isn't passing though. I think it's related to coverage having dropped.

BenLorantfy · 2019-10-25T12:37:42Z

Oh, I think it dropped because I removed a lot of code that was well tested, I'll see if I can add some more tests elsewhere.

julienben · 2019-10-25T12:50:55Z

I haven't taken a deep look at this PR's changes yet but maybe also check if some of the tests you removed were testing code outside the injectors?

coveralls · 2019-10-25T13:04:07Z

Coverage remained the same at 100.0% when pulling 631bcda on BenLorantfy:move-injectors-to-library into 9afffea on react-boilerplate:dev.

BenLorantfy · 2019-10-25T13:06:55Z

I checked and I don't think I removed any tests that were testing other code. I did remove some configureStore tests but the coverage for that file is still 100%.

I fixed the coverage by adding some tests for LanguageProvider and Loadable. Also copied the folder app/containers/LanguageProvider/tests to internals/templates/containers/LanguageProvider/tests because it was missing previously.

julienben · 2019-10-25T13:21:49Z

Thanks!!

Just one thing: Can you undo the copying of app/containers/LanguageProvider/tests? This is already handled by the cleaning script (lines 17 to 20).

BenLorantfy · 2019-10-25T13:36:48Z

Ah I see, done

julienben

Hey! Looks awesome overall. Just left a few small comments.

Also:

Make sure you merge the latest changes from dev and fix the conflicts. (Sorry, I merged a few PRs over the weekend 😅)
Did you check if code-splitting/loadable still works the same? I'm just worried that we lost that feature partially since the containers import from redux-injectors and the store setup does too. To test it, compare the webpack output of a build from the dev branch and your branch. They won't be exactly the same but you should see one file for each container. (Use webpack analyze if necessary.)
Any comments on files in the app folder also apply to the internals of course.

One more thing you might want to do is run a regression test as described in deployment.md. Just make sure you uncomment the git commit line in clean.js.

julienben · 2019-10-27T12:10:04Z

  }

  const sagaMiddleware = createSagaMiddleware(reduxSagaMonitorOptions);
+  const runSaga = sagaMiddleware.run;


Slightly prettier maybe:

const { run: runSaga } = sagaMiddleware;

julienben · 2019-10-27T12:15:45Z

-      expect(typeof store.runSaga).toBe('function');
-    });
+  it('should return a redux store', () => {
+    const store = configureStore({}, browserHistory);


Here browserHistory is undefined because it's an old obsolete API from react-router. You'll see it's been removed in #2791. You'll get a merge conflict here. You can just remove that argument and the test should pass just fine without it.

julienben · 2019-10-27T12:19:41Z

-    });
+  it('should return a redux store', () => {
+    const store = configureStore({}, browserHistory);
+    expect(typeof store).toBe('object');


It's a bit light to say it should return a redux store but then just check that it's of type object. You should use some of the same checks that used to be in checkStore. And IMO there's nothing wrong with also checking for the presence of the keys which are added by redux-injectors (runSaga, injectedSagas, injectedReducers, and replaceReducer too or is that gone now?).

So I added in a more robust test for checking if it returned a redux store. I'm hesitant to check for injectedSagas, injectedReducers, etc, because I'd consider that an internal implementation detail of redux-injectors. I'd like to be able to change that in the future if we need to without breaking react-boilerplate tests.

julienben · 2019-10-27T12:34:02Z

-1.  Remove `expect(() => checkStore({ ...store, injectedSagas: null })).toThrow();`
-2.  Remove `expect(() => checkStore({ ...store, runSaga: null })).toThrow();`
+4.  Remove statement `const runSaga = sagaMiddleware.run;`
+5.  Remove `runSaga` from `createInjectorsEnhancer` params


Does this actually work? You can use redux-injectors without redux-saga?

kind of. It throws some invariant warnings. I could probably disable them in redux-injectors

Could it be done by checking which keys are passed to createInjectorsEnhancer? It can be like a configuration object for the injectors and will be especially useful later if/when you add more integrations to them.

(No emergency here. We can merge this branch without this being present in injectors.)

Possibly, I'd have to look into it more

No rush. Just consider it for a future release of injectors.

julienben · 2019-10-27T12:37:51Z

+# `redux-injectors`
+
+`redux-injectors` is a library we made ourselves by moving code we used to have
+in the utils folder into a seperate library. It allows you to dynamically load


I would replace the first sentence with something like this:

redux-injectors is an official react-boilerplate companion library. We built it so that it can be used and maintained independently from react-boilerplate.

It allows you to dynamically load...

It feels slightly more professional IMO.

BenLorantfy · 2019-10-27T17:14:55Z

Looks like code-splitting is still working, I can see FeaturePage and HomePage are in two different files in the webpack output:

Regression test:

Example App:

Browse example app on development server
- Browse Features page, change language to de
- Browse NotFound page
Browse example app on dev tunnel
Browse example app on Production server
Browse example app offline

Internal Commands:

npm run clean (Be careful, this will commit all your changes.)
npm run generate component TestComp /w defaults
npm run generate container TestPage /w defaults

julienben

@BenLorantfy it's been a pleasure working with you on this. Thanks for your patience and your attention to detail! I hope you'll continue contributing!

BenLorantfy · 2019-10-27T19:27:58Z

No problem, thanks for your guidance and help with this 👍

lock · 2019-11-26T19:55:48Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

BenLorantfy mentioned this pull request Sep 23, 2019

Move reducer/saga injection to separate library #2698

Closed

Move injectors to library

e0cdec1

BenLorantfy force-pushed the move-injectors-to-library branch from eceacab to e0cdec1 Compare October 24, 2019 15:03

BenLorantfy marked this pull request as ready for review October 24, 2019 15:05

BenLorantfy requested review from JohnMostlyR, julienben, justingreenberg, jwinn and vineyardbovines as code owners October 24, 2019 15:05

BenLorantfy changed the title ~~[Please do not review yet] Move injectors to library~~ Move injectors to library Oct 24, 2019

BenLorantfy changed the title ~~Move injectors to library~~ Move injectors code to external library Oct 24, 2019

Add some tests to increase code coverage

11d5ceb

Undo copy language provider tests

e498d45

Merge branch 'dev' into move-injectors-to-library

db64281

julienben requested changes Oct 27, 2019

View reviewed changes

BenLorantfy added 2 commits October 27, 2019 12:06

Fix merge conflicts

006940d

Misc improvements

5fffec0

julienben requested changes Oct 27, 2019

View reviewed changes

BenLorantfy added 4 commits October 27, 2019 13:48

Update docs and remove extra files

01af83a

Add link

075cc3d

Fix link

e97d31c

Fix remove step

631bcda

julienben approved these changes Oct 27, 2019

View reviewed changes

julienben merged commit 2059424 into react-boilerplate:dev Oct 27, 2019

lock Bot locked as resolved and limited conversation to collaborators Nov 26, 2019

Uh oh!

Conversation

BenLorantfy commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienben commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenLorantfy commented Sep 22, 2019

Uh oh!

BenLorantfy commented Oct 24, 2019

Uh oh!

BenLorantfy commented Oct 24, 2019

Uh oh!

julienben commented Oct 25, 2019

Uh oh!

BenLorantfy commented Oct 25, 2019

Uh oh!

julienben commented Oct 25, 2019

Uh oh!

coveralls commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenLorantfy commented Oct 25, 2019

Uh oh!

julienben commented Oct 25, 2019

Uh oh!

BenLorantfy commented Oct 25, 2019

Uh oh!

julienben left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenLorantfy Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BenLorantfy commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression test:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienben left a comment

Choose a reason for hiding this comment

Uh oh!

BenLorantfy commented Oct 27, 2019

Uh oh!

lock Bot commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

BenLorantfy commented Sep 19, 2019 •

edited

Loading

julienben commented Sep 22, 2019 •

edited

Loading

coveralls commented Oct 25, 2019 •

edited

Loading

julienben left a comment •

edited

Loading

BenLorantfy Oct 27, 2019 •

edited

Loading

BenLorantfy commented Oct 27, 2019 •

edited

Loading