Add base template by BenLorantfy · Pull Request #1 · reduxjs/cra-template-redux

BenLorantfy · 2019-12-10T15:01:31Z

Creates a base template using @reduxjs/toolkit.

Started with cra default template and made some changes:

Added @reduxjs/toolkit, and react-redux as dependencies
Added "ducks" folder with one duck for a counter state
Added store.js that creates the store with the counter slice
Changed index.js to wrap App with Provider
Added a counter UI to demonstrate dispatching and selecting from the counter slice
Changed react branding to redux branding
Setup linting/prettier/testing for internal development (not published with template)

A few open questions I have:

Any best practices I'm missing?
Are the exports for the counter slice are done properly?
Is there any other opinions we want to bake into the template?
Do we want the "Learn Redux" link to link to redux.js.org or redux-toolkit.js.org ?

Screenshot of generated app:

markerikson · 2019-12-10T19:32:49Z

First observation based on the description: I'd prefer to have the folder named features instead of ducks.

Also, I'd like to consider moving App.js and store.js into a src/app folder as well, similar to these examples:

https://github.com/reduxjs/rtk-github-issues-example

https://redux.js.org/style-guide/style-guide#structure-files-as-feature-folders-or-ducks

nickserv · 2019-12-10T19:34:36Z

@markerikson I think moving App.js to a directory other than src could be confusing migrating from the default CRA template, but other than that I like using features and being consistent with the docs/examples

BenLorantfy · 2019-12-10T20:27:20Z

@nickmccurdy That's probably a good idea

nickserv · 2019-12-12T03:37:36Z

@BenLorantfy I've created a PR (#2). If you'd like to add me to your fork, I can help you merge the default template into it.

BenLorantfy · 2019-12-12T04:00:23Z

No worries, I can handle rebasing it after #2 get's merged 🙂

nickserv · 2019-12-12T04:01:58Z

I’m working on a code review, I’m not sure if rebasing preserves that

BenLorantfy · 2019-12-12T04:04:07Z

ah ok I can merge instead then

nickserv · 2019-12-12T04:15:56Z

Do we want the "Learn Redux" link to link to redux.js.org or redux-toolkit.js.org?

I'd like to link to Redux, Redux Toolkit, and React Redux in that order. Note that Redux Toolkit doesn't provide React bindings, and the docs don't seem to provide an introduction to Redux for new users.

nickserv

Thanks for your help! Here are my thoughts so far.

nickserv · 2019-12-12T03:55:09Z

+    "@testing-library/jest-dom": "^4.2.4",
+    "@testing-library/user-event": "^7.1.2",
+    "react-redux": "^7.1.3",
+    "@reduxjs/toolkit": "^1.1.0"


@reduxjs/toolkit should be first to keep consistent order with npm install, so installing new packages doesn't create a noisier diff.

nickserv · 2019-12-12T03:56:34Z

@@ -0,0 +1,68 @@
+This project was bootstrapped with [Create React App](https://github.com/facebook/create-react-app).


Maybe we should link to the Redux and/or Toolkit docs at the top. Should still link to the create-react-app docs and keep the rest of the readme though, as it's still relevant.

nickserv · 2019-12-12T04:08:55Z

+.App-counter {
+  display: flex;
+  align-items: center;
+  /* margin-top: 14px; */


We should either uncomment or remove this.

nickserv · 2019-12-12T04:10:00Z

+
+const { actions } = slice;
+
+export { actions, selectors, slice };


Personally I find separate export lines easier to read.

I would prefer simplicity and convention in a template.

Done. Should I be exporting an object of selectors or individually exporting each selector? Same for actions?

nickserv · 2019-12-12T04:11:30Z

+  "name": "cra-template-redux",
+  "version": "1.0.0",
+  "scripts": {
+    "prettify": "prettier './template/src/**/*.js' --write",


Maybe we should use CRA's Prettier config so there's less noise if someone switches from the default template.

They don't have a config file but I added the same flags they use here: https://github.com/facebook/create-react-app/blob/master/package.json#L19

nickserv · 2019-12-12T04:12:32Z

+  }
+});
+
+export default store;


We could have export default configureStore(...).

nickserv · 2019-12-12T04:31:49Z

+  );
+  const linkElement = getByText(/learn redux/i);
+  expect(linkElement).toBeInTheDocument();
+});


This could be a good opportunity to show an example of testing a simple Redux app, since it's more interactive than the default template. If we're just sticking with a basic smoke test this is fine with me.

@markerikson any thoughts on this? I can do a more in-depth test if we want.

Something like so? https://testing-library.com/docs/example-react-redux

I have a suggestion:

<span data-testid="count-value" className="App-counter-value"> {count} </span>

const setup = () => render( <Provider store={store}> <App /> </Provider> ); test('renders learn redux toolkit link', () => { const { getByText } = setup(); const linkElement = getByText(/redux toolkit/i); expect(linkElement).toBeInTheDocument(); }); test('can increment value', () => { const { getByTestId, getByText } = setup(); fireEvent.click(getByText('+')); expect(getByTestId('count-value')).toHaveTextContent('1'); }); test('can decrement value', () => { const { getByTestId, getByText } = setup(); fireEvent.click(getByText('-')); expect(getByTestId('count-value')).toHaveTextContent('0'); });

I also believe it's worth to add tests for the reducer as well:

// src/features/counter.test.js import counter, { actions } from './counter'; test('can increment value', () => { expect( counter( { value: 1 }, { type: actions.increment.type, } ).value ).toEqual(2); }); test('can decrement value', () => { expect( counter( { value: 2 }, { type: actions.decrement.type, } ).value ).toEqual(1); });

I can add something like that if we decide we want to add more in-depth testing.

luismasg · 2019-12-13T01:49:33Z

could we add a prepare function somewhere?
And a thunk ?
I could whip up a jsonplaceholder -ish PR
could this be a good opportuniy to recommend how to handle NOT_STARTED , LOADING and ERROR states in an action ?

nickserv · 2019-12-20T06:18:32Z

Now that the default template (#2) is merged, we can merge master back into this branch.

BenLorantfy · 2019-12-20T13:35:45Z

First observation based on the description: I'd prefer to have the folder named features instead of ducks.

Should I also move the counter UI to the counter feature folder? (and turn counter.js into a folder?)

leocaseiro · 2020-01-01T00:35:26Z

+    </Provider>
+  );
+
+  expect(getByText(/learn/i)).toBeInTheDocument();


Maybe search for: /react redux/i instead of just /learn/i?

expect(getByText(/react redux/i)).toBeInTheDocument();

BenLorantfy · 2020-01-04T02:08:20Z

@markerikson @nickmccurdy FYI, this is ready for another review 🙂

markerikson · 2020-01-05T22:08:39Z

    "template.json"
-  ]
+  ],
+  "devDependencies": {


Where in the template process are these devDeps actually used? Why isn't most of this already handled as part of CRA / react-scripts itself?

These dependencies don't end up in the project created by cra. These are just used by us to lint and test the code during development.

markerikson · 2020-01-05T22:09:35Z

+  align-items: center;
+}
+
+.App-counter-value {


I feel like this UI-specific CSS should be moved into a CSS modules file.

markerikson · 2020-01-05T22:11:01Z

    <div className="App">
      <header className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
+        <p className="App-counter">


Let's move all the counter UI into a <Counter /> component inside of features/counter.

markerikson · 2020-01-05T22:11:42Z

@@ -0,0 +1,25 @@
+import { createSlice } from '@reduxjs/toolkit';


I'd like this file to be moved to features/counter/counterSlice.js.

markerikson · 2020-01-05T22:12:52Z

+
+const selectCount = state => state.counter.value;
+
+export const selectors = {


I don't think exporting the selector inside of an object helps here. Just do export const selectCount =.

For that matter, does the selector really buy us anything useful in terms of the example? It at least shows the idea of a selector, but it also doesn't show use of Reselect.

I always like using selectors even for small things, since it hides the data structure from the consumers and therefore is easier to change. IMO the data structure of the state should be an implementation detail of the slice.

Up to you though.

markerikson · 2020-01-05T22:13:08Z

+  selectCount,
+};
+
+export const { actions } = slice;


Export the action creators individually here:

Suggested change

export const { actions } = slice;

export const { increment, decrement} = slice.actions;

markerikson · 2020-01-05T22:14:43Z

@@ -0,0 +1,8 @@
+import { configureStore } from '@reduxjs/toolkit';
+import * as slices from './features';


I don't like the import * here. I'd rather explicitly do import counterReducer from "features/counter/counterSlice".

Similarly, I'd rather not bother with trying to reuse the slice.name field. Just say counter: counterReducer.

BenLorantfy · 2020-01-19T05:45:13Z

@markerikson @nickmccurdy Sorry for the delay; this is ready for another review now.

markerikson · 2020-01-19T17:09:06Z

@@ -0,0 +1,31 @@
+.container {


In terms of naming convention, I've always preferred to make my CSS module files exactly match the name of the corresponding component files. In this case, that'd be capitalizing it to Counter.module.css.

markerikson · 2020-01-19T17:09:59Z

+  },
+  reducers: {
+    increment: state => {
+      state.value += 1;


Would it be worth adding an explanatory comment here saying that "Redux Toolkit allows us to 'mutate' the state"?

markerikson · 2020-01-19T17:11:10Z

+    increment: state => {
+      state.value += 1;
+    },
+    decrement: state => {


I'd like to see one other case handled here that actually shows using the action parameter as well. Maybe have an incrementByAmount case, and allow the user to enter a number in the UI?

BenLorantfy · 2020-01-28T03:42:37Z

@markerikson This is ready for another review 🙂

BenLorantfy · 2020-02-16T15:21:56Z

@markerikson Just pinging

markerikson · 2020-02-16T17:09:37Z

        <p>
          Edit <code>src/App.js</code> and save to reload.
        </p>
-        <a


Can we leave the "Learn React" link in here? That's over half of what the final app will be anyway :)

markerikson · 2020-02-16T17:12:08Z

@@ -0,0 +1,42 @@
+import React, { useState } from 'react';


Oh, one other naming scheme thing.

I'd like to change this to Counter.js.

I've never been a fan of the "using index.js for all the components based on folders" thing.

sure. Should the folder name be counter or Counter ?

features/counter/Counter.js, please

markerikson · 2020-02-16T17:12:27Z

Okay, I think I'm down to two nitpicks at this point.

markerikson · 2020-02-17T21:20:13Z

Awright, this is probably good enough to go with for now. Let's get this in!

Uh... what are the next steps? I assume we need to re-publish the package. I have access to the cra-template-redux package on NPM, so I guess it's up to me to do that?

BenLorantfy · 2020-02-17T21:21:24Z

Sounds good to me 👍

markerikson · 2020-02-17T21:24:05Z

Awright, I'll try publishing it this evening.

Would we want to call this 1.0.0?

BenLorantfy · 2020-02-17T21:27:25Z

I'd say so

nickserv · 2020-02-18T01:14:32Z

Let me know if you need help with docs or any other release stuff

markerikson · 2020-02-18T01:18:05Z

I published 0.0.1, and did find one bug: the CSS module import was now mismatched. Fixed that, published 0.0.2, created another CRA project with it, and that worked. Soooo... I've just published 1.0.0 !

https://github.com/reduxjs/cra-template-redux/releases/tag/v1.0.0

markerikson · 2020-02-18T01:27:51Z

The next step would be to tackle https://github.com/reduxjs/cra-template-redux-typescript . We should be able to piece it together by starting from the CRA-TS base template, copying over the code contents from this template, and TS-ifying it.

@BenLorantfy , you want to tackle that one too?

BenLorantfy · 2020-02-18T03:55:09Z

Sure I can take a shot at it

nickserv · 2020-02-20T03:29:36Z

Just read through the template again and I think it's a solid overview of basic CRA and Redux Toolkit functionality, nice work!

BenLorantfy added 6 commits December 10, 2019 09:06

Add template folder

d339079

Setup and fix tests not working

65f60b7

Change button style

45e75a2

Change counter store structure

ee8308c

Update assets

f48ca06

Update package.json

dae5c59

BenLorantfy mentioned this pull request Dec 10, 2019

Investigate creating a CRA template package reduxjs/redux#3650

Closed

This comment has been minimized.

Sign in to view

nickserv mentioned this pull request Dec 12, 2019

Add default template #2

Merged

nickserv suggested changes Dec 12, 2019

View reviewed changes

BenLorantfy added 7 commits December 20, 2019 08:14

Merge master

f1db627

Fix conflict

0cf6784

Update prettier command to use same flags as cra

d4c5040

Alphabetize deps

38abeac

Remove commented out css

45964a1

Change to inline exports

8b369a6

More concise export syntax for store.js

36e5e8c

BenLorantfy added 2 commits December 20, 2019 08:38

Add more links to different docs

c8231f0

Rename ducks to features

cef124a

leocaseiro reviewed Jan 1, 2020

View reviewed changes

markerikson suggested changes Jan 5, 2020

View reviewed changes

BenLorantfy added 2 commits January 19, 2020 00:31

Code review changes

a8020aa

Change to CSS modules

6c6c49b

markerikson suggested changes Jan 19, 2020

View reviewed changes

Implement code review suggestions

58776a0

BenLorantfy requested a review from markerikson January 28, 2020 03:42

BenLorantfy added 2 commits January 27, 2020 22:44

Fix typo

a9de675

Misc UX and styling fixes

e2b7db1

markerikson suggested changes Feb 16, 2020

View reviewed changes

markerikson reviewed Feb 16, 2020

View reviewed changes

Rename file and add back learn react link

4a37f28

BenLorantfy requested a review from markerikson February 17, 2020 21:15

Run prettier

e3bfce0

markerikson merged commit ff00b3d into reduxjs:master Feb 17, 2020

markerikson mentioned this pull request Feb 19, 2020

Create Redux CRA+TS template reduxjs/redux-templates#1

Closed

		@@ -0,0 +1,68 @@
		This project was bootstrapped with [Create React App](https://github.com/facebook/create-react-app).


		const { actions } = slice;

		export { actions, selectors, slice };

		@@ -0,0 +1,25 @@
		import { createSlice } from '@reduxjs/toolkit';


		const selectCount = state => state.counter.value;

		export const selectors = {

	export const { actions } = slice;
	export const { increment, decrement} = slice.actions;

		@@ -0,0 +1,8 @@
		import { configureStore } from '@reduxjs/toolkit';
		import * as slices from './features';

Conversation

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

Uh oh!

Uh oh!

markerikson commented Dec 10, 2019

Uh oh!

nickserv commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

BenLorantfy commented Dec 10, 2019

Uh oh!

nickserv commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenLorantfy commented Dec 12, 2019

Uh oh!

nickserv commented Dec 12, 2019

Uh oh!

BenLorantfy commented Dec 12, 2019

Uh oh!

nickserv commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickserv 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!

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!

leocaseiro Jan 1, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

luismasg commented Dec 13, 2019

Uh oh!

nickserv commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenLorantfy commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenLorantfy commented Jan 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenLorantfy Jan 5, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

BenLorantfy commented Dec 10, 2019 •

edited

Loading

nickserv commented Dec 10, 2019 •

edited

Loading

nickserv commented Dec 12, 2019 •

edited

Loading

nickserv commented Dec 12, 2019 •

edited

Loading

leocaseiro Jan 1, 2020 •

edited

Loading

nickserv commented Dec 20, 2019 •

edited

Loading

BenLorantfy Jan 5, 2020 •

edited

Loading