Skip to content
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

[Feature] Update Redux, Add Devtools #89

Merged

Conversation

hydrosquall
Copy link
Contributor

Redux Devtools gives us more granular views into the state of the redux store than console logs.

We will also get a minor performance boost from upgrading to the current version of react-redux (7), released last month.

This PR sits on top of commits from #86 and #87.

Preview deploy:

@micahstubbs
Copy link
Collaborator

nice thinking, redux devtools are great.

could you ensure that this one is either

  1. standalone, only redux devtools changes
  2. or only depends on [dependencies] Update to React 16, Webpack 4, Babel 7 #88

?

src/store.js Outdated
// Only instrument in dev environment
if (process.env.NODE_ENV !== "production") {
middlewares.push(createLogger);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hydrosquall curious, what is your reasoning for including redux logger too? I usually only use redux-devtools, and look at it's actions feed for the information that I got from redux logger before. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good question. Initially it was just something I'd reflexively included because the early projects I followed used it. Later on, I've found it useful to have some form of Redux introspection capabilities when debugging cross-browser issues in browsers without Redux devtools like IE / Safari. I'm not strongly attached to it though, and happy to drop it from the PR if it unnecessarily complicates things.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good to understand. let's add just Redux Devtools with this PR (drop redux-logger for now). Once we have that and we get #88 in, this one should be good to go.

and yes, committing our bundle does introduce a little more dev friction. thanks for bearing with it!

@hydrosquall
Copy link
Contributor Author

hydrosquall commented May 16, 2019

I'll wait to rebase this PR until #88 goes through, to avoid repeat rounds of regenerating the bundle.js / yarn.lock files.

@hydrosquall hydrosquall force-pushed the feature/upgrade-redux-devtools branch from f165c6b to 1a1d343 Compare May 19, 2019 15:58
@hydrosquall
Copy link
Contributor Author

hydrosquall commented May 19, 2019

I dropped the commit with redux-logger, I will wait on recommitting the bundle.js until #88 is merged.

@hydrosquall hydrosquall changed the title [Feature] Update Redux, Add Devtools + Logger [Feature] Update Redux, Add Devtools May 23, 2019
@micahstubbs
Copy link
Collaborator

micahstubbs commented May 23, 2019

@hydrosquall here's a PR into your branch to resolve the merge conflicts hydrosquall#2

.babelrc Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@micahstubbs
Copy link
Collaborator

@hydrosquall ok, let's do this one next. sent a PR and added some comments. progress!

@hydrosquall hydrosquall force-pushed the feature/upgrade-redux-devtools branch from 1a1d343 to 4e45873 Compare May 24, 2019 00:59
@hydrosquall hydrosquall force-pushed the feature/upgrade-redux-devtools branch from 4e45873 to c06c35a Compare May 24, 2019 01:01
@hydrosquall
Copy link
Contributor Author

I rebased the branch so that the git history is clean, and ensured that the dependency upgrades (redux, react-redux, and redux-thunk) were all made relative to the yarn.lock of the current master branch. This is ready to recheck :).

@micahstubbs
Copy link
Collaborator

nice, thanks for rebasing and fixing the conflicts! will take another look.

@micahstubbs
Copy link
Collaborator

ok, diff looks good, but I see an error when I test this locally

Screen Shot 2019-05-24 at 7 29 24 PM

@hydrosquall
Copy link
Contributor Author

hydrosquall commented May 25, 2019 via email

@micahstubbs
Copy link
Collaborator

ah, nevermind I know why this is. let me try on the other machine that has the secrets.json configured to use the remote production search backend 💡

@micahstubbs
Copy link
Collaborator

@hydrosquall as you might have noticed, this config in secrets.json is a pain point with our current dev setup 😅

@micahstubbs
Copy link
Collaborator

progress. now that I add the secrets.json config to use the remote search backend, I see the search results render

Screen Shot 2019-05-24 at 9 42 08 PM

@micahstubbs
Copy link
Collaborator

that said, redux-devtool doesn't see a store. instead, it points us to this help link https://github.com/zalmoxisus/redux-devtools-extension#usage

Screen Shot 2019-05-24 at 9 44 49 PM

@micahstubbs
Copy link
Collaborator

going to update my redux-devtools Chrome extension and see if that makes a difference https://github.com/zalmoxisus/redux-devtools-extension#installation

@micahstubbs
Copy link
Collaborator

ok, updating to https://github.com/zalmoxisus/redux-devtools-extension/releases/tag/2.17.0 did not fix it. must be something else 🤔

@micahstubbs
Copy link
Collaborator

I think I found the problem. webpack is defaulting to production mode

WARNING in configuration
The 'mode' option has not been set, webpack will fallback to 'production' for this value. Set 'mode' option to 'development' or 'production' to enable defaults for each environment.
You can also set it to 'none' to disable any default behavior. Learn more: https://webpack.js.org/configuration/mode/

@micahstubbs
Copy link
Collaborator

we can set the mode to development in the yarn scripts in package.json

for example:

"buildWatch": "webpack --color --progress --watch --mode=development",

@micahstubbs
Copy link
Collaborator

going to pause here, and come back to this later. so weird that Redux devtools don't see the store.

@hydrosquall
Copy link
Contributor Author

hydrosquall commented May 25, 2019

Hm, Redux Devtools will work in both development and production mode, I just rechecked using the "docker compose" local setup in development mode.

And then there's the deployed code in production mode, just to confirm that everything is OK with the devtools extension.

Is it possible that we might be loading a cached version of the page, where the old code didn't use the alternate compose function? One way I check this sometimes is going to the "sources" tab, hitting "ctrl+p", and checking the file of interest (in this case, store.js to ensure we're using the new compose function.)

@micahstubbs
Copy link
Collaborator

hah! @hydrosquall we should get you setup with the same local dev environment (pre-docker) so that we can compare with all other things equal.

I'll DM you the secrets.json file that you need for local dev with a remote search backend for now.

@micahstubbs
Copy link
Collaborator

I'm definitely seeing the old store.js. odd. In Chrome devtools, I went to Applications --> Clear Site Data. thought that would do it. I also have Network --> disable cache checked. 🤔

Screen Shot 2019-05-25 at 3 21 57 PM

@micahstubbs
Copy link
Collaborator

fixed it 💡

cd blockbuilder
yarn link blockbuilder-search
yarn build
node server.js

somehow my yarn link had gone stale. weird.

Screen Shot 2019-05-25 at 3 26 26 PM

@micahstubbs
Copy link
Collaborator

just discovered this unrelated issue #94

Copy link
Collaborator

@micahstubbs micahstubbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks for the contribution!

@micahstubbs micahstubbs merged commit 3b7676a into enjalot:master May 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants