Setup tooling and add docs #2

BenLorantfy · 2019-08-29T16:57:14Z

I think this is all that's left :)

In this PR:

Setup bili for cjs and es builds
- Not sure if I also need umd or if cjs and es is enough
Setup prettier
Setup eslint
- Copied linting rules from rbp
- Disabled a few rules that would cause a different style than rbp. I think this is because I installed the latest airbnb preset, rbp might be on an earlier one.
- Removed jsx-a11y since we don't render any dom elements
Setup lint-staged
Created travis file
- Need help enabling travis for this repo though
Add "prepublishOnly" script that runs the tests and build before publish
Add "files" array to package.json
Add docs

julienben · 2019-09-02T15:47:02Z

Trying to review this during the week. Will update you. 👌

julienben

I have a few small comments but overall this looks great.

If I have only thing to say, it would be that I'm not 100% clear on the README. I wish it did a better job of explaining to potential new users what this is for.

julienben · 2019-09-05T15:28:56Z

README.md

+)
+```
+
+Note the `createInjectorsEnhancer` function takes two options. `createReducer` should be a function that when called will create the root reducer. It's passed the injected reducers as an object of key-reducer pairs.


"will create the root reducer" => Might want to write "will return the root reducer", no?

julienben · 2019-09-05T15:29:38Z

README.md

+}
+```
+
+`runSaga` should ussually be `sagaMiddleware.run`. 


Ussually should be replaced in a few places.

julienben · 2019-09-05T15:30:57Z

README.md

+
+## Motivation
+There's a few reasons why you might not want to load all your reducers and sagas upfront:
+1. You don't need all the reducers and sagas for every page. This library let's you only load the reducers/sagas that are needed for the page being viewed. This speeds up the page load time because you can take advantage of [code-splitting](https://webpack.js.org/guides/code-splitting/).  This is also good for performance after the page has loaded, because less reducers and sagas are running. 


Spelling:

This library lets you load

Less reducers => Fewer reducers

(Similar let's => lets errors in other places)

README.md

julienben · 2019-09-05T15:35:09Z

src/checkStore.js


 /**
- * Validate the shape of redux store
+ * Validates the redux store is setup properly to work with this library.


setup => set up (it's used as a verb here, not a noun)

BenLorantfy · 2019-09-05T22:26:48Z

I implemented all of the requested changes. I'm not exactly sure how to improve the README. I tried re-wording the beginning a bit but if that's not clear enough I might need help.

Also, I need help enabling travis to work on this repo. I think someone needs to go into travis settings and enable it.

julienben · 2019-09-08T11:07:12Z

This is what Travis tells me when I try to enable it for this repo. I'm assuming this means we need a travis file and some tests in the master branch before it can be enabled?

BenLorantfy · 2019-09-08T16:16:08Z

ah you're probably right. We can do that after it gets into master.

BenLorantfy · 2019-09-13T23:14:02Z

@julienben Is this PR good to go or is there anything else I should change?

julienben

A few additional thoughts. Will continue reviewing later.

julienben · 2019-09-15T09:53:44Z

README.md

@@ -1,2 +1,81 @@
 # injectors
-Asynchronous injectors for Redux reducers and sagas. As used by react-boilerplate.
+Dynamically load [redux](https://redux.js.org/) reducers and [redux-saga](https://redux-saga.js.org/) sagas as needed, instead of loading all of them upfront. See [motivation](#Motivation). As used by react-boilerplate.


Maybe link to react-boilerplate's repo in "as used by rbp"?

I would also consider adding a line re:motivation instead of having everything at the bottom.

julienben · 2019-09-15T10:01:44Z

README.md

+```
+
+### Setting up the redux store
+The redux store needs to be configured to allow this library to work. The library exports a store enhancer that can be passed to the `createStore` function.


Here's where the explanation could be clearer IMO. Instead of simply "the store needs to be configured for the library to work", something more explicit like:

We take advantage of the redux store being a simple POJO to add to it several keys:

injectedReducers: A registry of all the reducers injected into the app to ensure we don't overwrite an existing reducer.

injectedSagas: Same thing but for sagas. Also makes sure we don't run the same saga twice for one action.

runSaga: Just like redux provides store.replaceReducer, we need to add store.runSaga to enable the dynamic injection mechanism.

That's kind of getting into implementation details. Do we need to explain that in the "Setting up the redux store" section? It's meant to be a quick start so I kind of wanted to be brief.

I understand it's helpful to explain this so developers don't think it's magic, but I wonder if this could go in a different section? i.e. "How does it work?"

Sounds good!

julienben · 2019-09-15T10:04:18Z

README.md

+
+const store = createStore(
+ createReducer(),
+ undefined,


Why not write initialState here?

julienben

Looks excellent!

julienben · 2019-09-16T07:55:45Z

docs/api.md

+
+const store = createStore(
+ createReducer(),
+ undefined,


initialState would be nice here too.

julienben · 2019-09-16T07:56:42Z

docs/api.md

+import { createInjectorsEnhancer } from "injectors"
+
+function createReducer(injectedReducers = {}) {
+ const rootReducer = combineReducers({


Indentation is off in some of your code samples. 0 => 1 space => 3 spaces

julienben · 2019-09-16T07:57:50Z

docs/api.md

+
+#### Parameters
+
+-   `options` **[Object][24]** 


Do you want to call this "options" given that they're not actually optional?

Or is that just a standard pattern?

re-named to params

Maybe do it everywhere that function params are not optional? There's a few more instances apparently.

ah I forgot to re-build the docs, I'll fix that after work

julienben · 2019-09-16T16:23:14Z

(Btw, those were my final comments. Let's merge asap, do anything else that needs to be done before publishing - like picking a name!, and then PR to react-boilerplate.)

BenLorantfy added 13 commits August 29, 2019 12:56

Add travis file

6969657

Setup linting and prettier

77b99fb

Pin depends, add prepublish, add files

24736ad

Add lint to travis

74295ad

Add docs

1e32bbe

Add CONTRIBUTING.md, misc cleanup

d2aca41

Update README.md

a6cfe06

Fixes

1fba4ab

Organize dosc

dc41f04

Fix

96bd56d

Update api.md

59f2a02

Fix

0592334

Fixes

406945b

BenLorantfy changed the title ~~Please do not review yet~~ Setup tooling and add docs Sep 1, 2019

BenLorantfy marked this pull request as ready for review September 1, 2019 20:48

BenLorantfy requested a review from julienben September 1, 2019 20:49

BenLorantfy mentioned this pull request Sep 1, 2019

Move reducer/saga injection to separate library react-boilerplate/react-boilerplate#2698

Closed

julienben requested changes Sep 5, 2019

View reviewed changes

BenLorantfy added 2 commits September 5, 2019 18:03

Fix spelling and misc

30798ce

re-word README beginning

0125f7a

julienben requested changes Sep 15, 2019

View reviewed changes

README improvements

b6073e1

julienben reviewed Sep 16, 2019

View reviewed changes

misc fixes

6d7de7b

julienben approved these changes Sep 16, 2019

View reviewed changes

bulid docs

4ecaeff

BenLorantfy merged commit 3223506 into dev Sep 16, 2019

julienben deleted the setup-processes branch September 25, 2019 13:38

Setup tooling and add docs #2

Setup tooling and add docs #2

Uh oh!

Conversation

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

Uh oh!

In this PR:

Uh oh!

julienben commented Sep 2, 2019

Uh oh!

julienben left a comment

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!

Choose a reason for hiding this comment

Uh oh!

BenLorantfy commented Sep 5, 2019

Uh oh!

julienben commented Sep 8, 2019

Uh oh!

BenLorantfy commented Sep 8, 2019

Uh oh!

BenLorantfy commented Sep 13, 2019

Uh oh!

julienben left a comment

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!

julienben left a comment

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!

Choose a reason for hiding this comment

Uh oh!

julienben commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BenLorantfy commented Aug 29, 2019 •

edited

Loading