Upgrade React & React-DOM & React-Router + adding DevTools #2990

AWolf81 · 2019-05-03T22:45:18Z

Great app. I really like the app and this is my first contribution to the code base. I think this is ready for a first review but I'll do a review tomorrow (e.g. to remove development console.logs if I've missed any etc.)

Description

I've updated React, React-DOM & React-Router to the latest version. There was no issue for this open but I think it's good to update & React-carousel complaint during installation that React was not matching the library version.

I've first thought this is easier todo but it was quite some work. The main problem was React-Router as the newest version v5 is totally different compared to v2 - biggest issue was the removed query prop from history. There is a good post about how to migrate & what's different compared to v2/v3
First I wanted to migrate to v3 but this was also not working out-of the box - so I decided to migrate the routing to the latest version.

So please have a close look at every route. I think I've tested everything but maybe I have missed an edge case.

Overview

Upgraded React, Redux, React-Router & some more package that were required to the latest version
Added React & Redux-Devtools (see screenshot below) - helped during development
Changed routing to component based routing as required by React-Router

Issues in the code

Sometimes during fast typing in search field the input will lose focus and move to markdown text area. Need to check what's causing this and if it's introduced by the routing. It's also happening on master so it's not introduced here. It happens during backspace deleting letters.
There are many warnings about styling issues in the console - we could turn the output off by ignoring them but I think it's better to fix them and keep the setting to logging. I haven't looked into this and I think we could improve this later.
There are warnings from React about missing props. I think I haven't introduced these but they're probably easy to fix either by removing isRequired or by not rendering if an important part is missing.
Initial rendering is not smooth but it is as in master. I think we can improve this later by delaying display until everything is ready & keep the loading spinner active. So the app loading is looking way nicer.

Possible improvements (later, no need to do it in this PR)

I think the redux part of the app could be refactored (separate reducer files, action creators & mapState & mapDispatchToProps with connected components) - if you like I can create an issue for this. This will improve readability / code quality.
Code style improvement: I think it would be good to use class properties instead of the binding in constructor. Less code & better readability. Like mentioned in this post.
Auto-scroll preformance (here & on master): There is a pretty large lag on Windows 10 during scrolling around 0.5 - 1s before scrolling starts. Not sure why & I need to check if I can see something in the profiling.

Issue fixed

Type of changes

⚪ Bug fix (Change that fixed an issue)
🔘 Breaking change (Change that can cause existing functionality to change)
🔘 Improvement (Change that improves the code. Maybe performance or development improvement)
⚪ Feature (Change that adds new functionality)
⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

🔘 My code follows the project code style
⚪ I have written test for my code and it has been tested
⚪ All existing tests have been passed - yarn jest passed but yarn test failed (see screenshot below) - need to check why it fails
⚪ I have attached a screenshot/video to visualize my change if possible

Failed yarn test

DevTools

Todos

Enable & fix auto-scrolling (commented because of the query to search change)

Rokt33r · 2019-05-08T03:15:27Z

@AWolf81 Submit this issue to here. https://issuehunt.io/r/BoostIo/Boostnote/issues/2993 I'm going to pay for this pr gladly.

Rokt33r · 2019-05-08T03:19:03Z

browser/lib/newNote.js

@@ -1,7 +1,9 @@
-import { hashHistory } from 'react-router'
+// import { hashHistory } from 'react-router'


Discard comments

Rokt33r · 2019-05-08T03:19:14Z

browser/main/Detail/MarkdownNoteDetail.js

@@ -9,7 +9,8 @@ import StarButton from './StarButton'
 import TagSelect from './TagSelect'
 import FolderSelect from './FolderSelect'
 import dataApi from 'browser/main/lib/dataApi'
-import { hashHistory } from 'react-router'
+// import { hashHistory } from 'react-router'


Discard comments

Rokt33r · 2019-05-08T03:19:19Z

browser/main/Detail/SnippetNoteDetail.js

@@ -8,7 +8,8 @@ import StarButton from './StarButton'
 import TagSelect from './TagSelect'
 import FolderSelect from './FolderSelect'
 import dataApi from 'browser/main/lib/dataApi'
-import {hashHistory} from 'react-router'
+// import {hashHistory} from 'react-router'


Discard comments

Rokt33r · 2019-05-08T03:19:27Z

browser/main/Main.js

@@ -12,8 +12,8 @@ import _ from 'lodash'
 import ConfigManager from 'browser/main/lib/ConfigManager'
 import mobileAnalytics from 'browser/main/lib/AwsMobileAnalyticsConfig'
 import eventEmitter from 'browser/main/lib/eventEmitter'
-import { hashHistory } from 'react-router'
-import store from 'browser/main/store'
+// import { hashHistory } from 'react-router'


Discard comments

browser/main/NewNoteButton/index.js

browser/main/store.js

Rokt33r · 2019-05-08T03:33:17Z

lib/main.html

@@ -132,8 +132,8 @@
  </script>

  <script src="../node_modules/@rokt33r/js-sequence-diagrams/dist/sequence-diagram-min.js"></script>
-  <script src="../node_modules/react/dist/react.min.js"></script>
-  <script src="../node_modules/react-dom/dist/react-dom.min.js"></script>
+  <script src="../node_modules/react/umd/react.development.js"></script>


I assume we need to use another html file for production env because I don't want to use development script in the compiled app.

So what I want to ask are:

Rename this file to lib/main.development.html

Create lib/main.production.html by copying from lib/main.development.html and update the url so we can use production script.

Update declaration statements for const url in lib/main-window.js so electron can choose the html files by environment.

Good catch. I've added this.
I've also updated the link regex or was that not needed?
What do you think should we create an issue to bundle the electron scripts and we don't have to manually import each script? Minifiying could be done by Webpack production build too.

I've also updated the link regex or was that not needed?

I cannot understand. Could you point where the regex is in?

What do you think should we create an issue to bundle the electron scripts and we don't have to manually import each script? Minifiying could be done by Webpack production build too.

I absolutely want to do. But let's do things one by one. It should be done in another pr.

OK, I think there is already issue #1561 for this. I can add some ideas I have as a comment to that issue.
First part of the todolist in that issue Upgrade React is done once this is merged.

browser/lib/newNote.js

AWolf81 · 2019-05-08T12:07:43Z

@Rokt33r thanks for the reward at IssueHunt.

I've addressed you review comments. That created a conflict with master. Can you please fix that during merging to master? I think I can not fix that.
I think it's just conflicting because I have renamed main.html.

Just the inline comment that I have added is open - should I modify this as I proposed? But it would be probably also OK to use the imported history as we're also having another location where we're doing it like this.
I've only thought that it should be easy as the function parameter dispatch is already available.
At the other location, I'm not sure if that component is connected to Redux but if not we could connect it and we're consitent with the usage.

browser/lib/newNote.js

browser/main/NoteList/index.js

AWolf81 · 2019-05-09T05:27:16Z

browser/components/MarkdownPreview.js

@@ -1023,7 +1023,8 @@ export default class MarkdownPreview extends React.Component {

    if (!href) return

-    const regexNoteInternalLink = /main.html#(.+)/
+    const regexNoteInternalLink = process.env.NODE_ENV === 'production' ? /main.production.html#(.+)/ : /main.development.html#(.+)/


This is the regex I've metioned.
After having a second look on this I think it's important to update. I tested the regex here.

Hmm.. /main.[A-Za-z0-9]+.html#/ might be better to have rather than two regex.

Yes, good idea. Maybe also /main[.\w]*.html#/ will work - also supports main.html. I'll update this later today.

https://regex101.com/r/zRLTIR/2

@AWolf81 Yeah I think your idea is better. Could you fix it too? Then I think we could merge this.

I think it's because there is no main.development.html in the linkHash or href string. Maybe new router changed here.
I have to check how to detect the local link.

Maybe I have the link in markdown not right. Do I need one slash in the url? I tested with [test link](#Test). I was a bit in a hurry before going to work. I'll check it later today after work.

I'll try it too later.

OK, I've fixed it by modifying parsing of the href string. Inspired by the following post.

Tested all link types (empty link, line link :line:<number>, tag link #title & note link :note:noteKey). I just couldn't verify old link style storage.key-note.key - not sure how to test it. But it should still work.

Note: Adding a slash to a tag link e.g. /#title won't work but I think that's OK - same behavior as on master.

I think ctrl+left mouse handling in the editor is not working for every link type, right? Tested on master. I think it's using just the open external Url link - it would be good to share the link handling code for this location so the same functionality is available there.

Should we create an issue for this so it can be improved later?

AWolf81 · 2019-05-09T05:53:25Z

@Rokt33r I've replaced the direct usage of history with dispatch to connected-react-router.

I think it would be good to add that we recommend to use dipatch(push(...)) over history.push(...) where ever possible. Maybe we could add this to code style guide doc. If you like I can create a PR for this.

In the style guide I've seen that it's recommended to use class property over class methods. What do you think should we clean our code base to be consitent with this? During my last reviews I commented to have .bind(this) as this is used almost everywhere.

I think we should also add some info to React hooks and how we'd like to use them.

Existing code will be kept class-based and will be only changed if it improves readability or makes things more reusable. For new components it's OK to use hooks & functional components but don't mix hooks & class-based within one feature - just for code style / readability reasons.
I don't have an example for mixing both but maybe I can create one.

Rokt33r · 2019-05-09T06:40:35Z

In the style guide I've seen that it's recommended to use class property over class methods. What do you think should we clean our code base to be consitent with this? During my last reviews I commented to have .bind(this) as this is used almost everywhere.

I think we should also add some info to React hooks and how we'd like to use them.

Both of idea sounds great. But let's do it in other prs. And, I'm going to merge this soon.

…ring

AWolf81 · 2019-05-10T22:36:13Z

browser/components/MarkdownPreview.js

-    if (regexNoteInternalLink.test(linkHash)) {
-      const targetId = mdurl.encode(linkHash.match(regexNoteInternalLink)[1])
-      const targetElement = this.refs.root.contentWindow.document.getElementById(
+    const regexNoteInternalLink = /.*[main.\w]*.html#/


Note
Added .* to the regex in front of [main.\w] to match everything before main.development.html. The string to test contains file protocol + path e.g. file:///c:/path/to/project/main.development.html

https://regex101.com/r/zRLTIR/4

AWolf81 · 2019-05-10T22:37:30Z

browser/components/MarkdownPreview.js

+
+    if (regexNoteInternalLink.test(href)) {
+      const targetId = mdurl.encode(linkHash)
+      const targetElement = this.refs.root.contentWindow.document.querySelector(


Changed to querySelector because targetId contains # e.g. #test

AWolf81 · 2019-05-11T09:44:06Z

package.json

-    "react-router-redux": "^4.0.4",
-    "react-test-renderer": "^15.6.2",
+    "react-test-renderer": "^16.8.6",
+    "redux-devtools": "^3.5.0",


There is an issue with redux-devtools. I'm not sure if we need to fix this before merging but I'm getting the error message from the screenshot pretty often especially if I'm selecting a dom element.

During install of redux-devtools I'm getting the following warning " > [email protected]" has incorrect peer dependency "react-redux@^4.0.0 || ^5.0.0 || ^6.0.0".
There is an issue to it here but with-out a comment. Maybe we have to change version to React-redux 6.x until this is fixed.

I can not reproduce the behaviour with the warning message at the moment. Is this still a problem? If not I think we can stay ar react-redux 7.x and ignore the warning from redux-devtools.

AWolf81 · 2019-05-25T06:06:00Z

@Rokt33r I've merged master into this branch & checked the issue with devtools but couldn't reproduce it.

If it occurs later I think changing React-redux to 6.x version could improve/fix the problem. But we should avoid using the old version as they're working on hooks support improvements.

So I think this is ready for merging or is anything open that I should check?

Rokt33r · 2019-05-25T08:12:37Z

So I think this is ready for merging or is anything open that I should check?

You don't need to. I'm going to merge this after I merge #2983

Seems lots of people don't like this feature so I'm going to merge tomorrow and deploy again.

Rokt33r · 2019-05-28T13:26:00Z

@AWolf81 Could you fix the conflicts? I'm going to merge this as soon as they are fixed.

AWolf81 · 2019-05-28T18:32:47Z

@Rokt33r Sure, I've merged master, did a quick test and everything is working.

AWolf81 added 3 commits May 3, 2019 23:54

Update dependencies & change to React-router v5

a6a522b

add React & Redux devtools

0fc4549

fix auto-scroll

496dd1b

ZeroX-DG requested a review from Rokt33r May 4, 2019 01:18

ZeroX-DG assigned Rokt33r May 4, 2019

ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels May 4, 2019

Rokt33r requested changes May 8, 2019

View reviewed changes

AWolf81 added 2 commits May 8, 2019 08:46

address review comments - add production/dev main.html & remove comments

546f2eb

remove commented imports

5b22e0f

AWolf81 commented May 8, 2019

View reviewed changes

browser/lib/newNote.js Outdated Show resolved Hide resolved

Rokt33r requested changes May 9, 2019

View reviewed changes

browser/lib/newNote.js Outdated Show resolved Hide resolved

browser/main/NoteList/index.js Outdated Show resolved Hide resolved

AWolf81 commented May 9, 2019

View reviewed changes

change redirecting to connected-react-router

3e8bb6a

AWolf81 mentioned this pull request May 9, 2019

Style guide doc update for React Hooks #2999

Closed

fix local link detection by creating a link tag to parse the input st…

e49f6fb

…ring

AWolf81 commented May 10, 2019

View reviewed changes

AWolf81 commented May 11, 2019

View reviewed changes

Merge branch 'master' into upgrade-react

4c9efbe

Merge branch 'master' into upgrade-react

ad7a5ff

Rokt33r merged commit 1afa02b into BoostIO:master May 29, 2019

Rokt33r removed the awaiting review ❇️ Pull request is awaiting a review. label May 29, 2019

Rokt33r added this to the v0.12.0 milestone May 29, 2019

roottool mentioned this pull request Jun 9, 2019

Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

Merged

AWolf81 deleted the upgrade-react branch July 5, 2019 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade React & React-DOM & React-Router + adding DevTools #2990

Upgrade React & React-DOM & React-Router + adding DevTools #2990

AWolf81 commented May 3, 2019 •

edited

Loading

Rokt33r commented May 8, 2019

Rokt33r May 8, 2019

Rokt33r May 8, 2019

Rokt33r May 8, 2019

Rokt33r May 8, 2019

Rokt33r May 8, 2019

AWolf81 May 8, 2019

Rokt33r May 9, 2019 •

edited

Loading

AWolf81 May 9, 2019 •

edited

Loading

AWolf81 commented May 8, 2019

AWolf81 May 9, 2019

Rokt33r May 9, 2019

AWolf81 May 9, 2019

AWolf81 May 9, 2019

Rokt33r May 10, 2019

AWolf81 May 10, 2019 •

edited

Loading

AWolf81 May 10, 2019

Rokt33r May 10, 2019

AWolf81 May 10, 2019

AWolf81 May 11, 2019

AWolf81 commented May 9, 2019 •

edited

Loading

Rokt33r commented May 9, 2019

AWolf81 May 10, 2019

AWolf81 May 10, 2019

AWolf81 May 11, 2019

AWolf81 May 25, 2019

AWolf81 commented May 25, 2019

Rokt33r commented May 25, 2019

Rokt33r commented May 28, 2019

AWolf81 commented May 28, 2019

		@@ -1,7 +1,9 @@
		import { hashHistory } from 'react-router'
		// import { hashHistory } from 'react-router'

Upgrade React & React-DOM & React-Router + adding DevTools #2990

Upgrade React & React-DOM & React-Router + adding DevTools #2990

Conversation

AWolf81 commented May 3, 2019 • edited Loading

Description

Issue fixed

Type of changes

Checklist:

Rokt33r commented May 8, 2019

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

Rokt33r May 9, 2019 • edited Loading

Choose a reason for hiding this comment

AWolf81 May 9, 2019 • edited Loading

Choose a reason for hiding this comment

AWolf81 commented May 8, 2019

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

AWolf81 May 10, 2019 • 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

AWolf81 commented May 9, 2019 • edited Loading

Rokt33r commented May 9, 2019

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

AWolf81 commented May 25, 2019

Rokt33r commented May 25, 2019

Rokt33r commented May 28, 2019

AWolf81 commented May 28, 2019

AWolf81 commented May 3, 2019 •

edited

Loading

Rokt33r May 9, 2019 •

edited

Loading

AWolf81 May 9, 2019 •

edited

Loading

AWolf81 May 10, 2019 •

edited

Loading

AWolf81 commented May 9, 2019 •

edited

Loading