Todomvc react update #98

flashdesignory · 2023-03-13T14:50:45Z

Update version to 17.0.2
Add updated readme
Enforce style guide.
Rewrite & cleanup app
Use functional components & hooks
Break up components into separate building blocks - MV(*)
Use Webpack 5 + Babel
Ensure Toggle for all todos is displaying correctly

numbers:
https://gist.github.com/flashdesignory/dfe0158371f7c6052dbfe4ba8767a96b

julienw

I looked at this commit by commit, and while it gave a good insight in your thought process, it didn't make it very easy to review. I believe that otherwise all my comments to the preact PR also apply here, especially related to the use of useCallback and useMemo, so I won't repeat them. In the commits I see that you introduced them after some feedback, so I'd love to know more :-)
In case you'd like to keep them, it would be good to document the rationale using comments in the code.

Compared to the preact PR, this time the numbers are much lower, have you profiled to understand what could have changed?

julienw · 2023-03-13T18:26:32Z

resources/todomvc/architecture-examples/react/src/todo/components/Input.jsx

+
+    return (
+        <form onSubmit={handleSubmit}>
+            <input className="new-todo" id="todo-input" type="text" data-testid="text-input" autoFocus placeholder={placeholder} defaultValue={defaultValue} onBlur={handleBlur} />


In react I believe it's usually advised to implement as a "controlled input", where the value is kept in the local state. Here instead you implemented an uncontrolled input. I believe this works and I'll leave it up to you to decide what style you prefer, but I wanted to point it out. For example a controlled component might make it easier to access the value in handleSubmit.

Controlled components: https://reactjs.org/docs/forms.html#controlled-components
Uncontrolled components: https://reactjs.org/docs/uncontrolled-components.html

This is the old doc, it looks like that the new documentation website is less assertive about which style to use.

yes - I used a form before 😄
It does solve some problems, but even with a form I honestly wouldn't keep local state.
I could still read the value from the event (e.target.elements["todo-input"].value).

flashdesignory · 2023-03-13T19:41:27Z

@julienw - I did and I am still comparing both the react and preact versions to determine what affects the scores.
It's hard to compare both previous builds, since they are very different.

Overall, the biggest impact in the new builds was to wrap the with a memo call, to ensure items that didn't change won't re-render. You can observe that behavior in both builds if you run them locally with the dev tools.

The react version went through some internal feedback rounds initially and therefore you can see some changes in the commits.

Regarding the inputs, I can see that there might be some discussions and I can switch the input to a controlled component if there are strong feelings. I honestly didn't see a need to keep additional state, if we can just read the value on the event handler.

julienw · 2023-03-14T14:35:24Z

resources/tests.mjs

-    name: "React-TodoMVC",
-    url: "todomvc/architecture-examples/react/index.html",
+    name: "TodoMVC-React",
+    url: "todomvc/architecture-examples/react/dist/index.html",
    async prepare(page) {
        const element = await page.waitForElement(".new-todo");
        element.focus();


This is a comment for the line dispatchEvent("input") below, and this can be handled in a separate pull request.

Current versions of react (since ~v16) have some code that tracks the value. As a result the input event doesn't do anything now:

when setValue is called, react saves the value

when the event is dispatched, react compares the current value with the saved one, they are the same value => as a result it swallows the input event. See dispatchEvent on input/textarea is ignored facebook/react#10135 for more discussion about this.

The solution that the testing-library library uses is this setNativeValue function that we might want to steal to replace our current implementation of setValue:
https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/events.js#L106-L123

This has no impact to this version of todomvc because this version doesn't use the input or change event (as opposed to the previous one), that's why it can be handled separely.

julienw · 2023-03-14T14:51:16Z

Today I tried to only update react to v17 on the old app. You can see the result in this branch => https://github.com/WebKit/Speedometer/compare/main...julienw:Speedometer:only-update-react?expand=1
(I didn't check in the changes to node_modules so make sure to run npm i if you want to try it -- also my editor automatically formatted the benchmark-runner so don't pay attention to that)

In short: in the context of this app, the React v17 update seems to be just a little bit faster on chrome, and similarly fast in Firefox. (I can't check Safari).
(update: I meant that Firefox was approximately as fast with React v17 than React v15, while Chrome seemed to be faster with React v17 than with React v15)

flashdesignory · 2023-03-14T15:03:57Z

@julienw - that's a great comparison.
How do you measure the speed and are the comparisons against the "old" react workload or the proposed react workload?

julienw · 2023-03-14T15:12:33Z

@julienw - that's a great comparison. How do you measure the speed and are the comparisons against the "old" react workload or the proposed react workload?

I compared on my machine (Xorg Linux Debian):

the old workload with react v15
the old workload with react v17
the new workload with react v17

With Chrome stable 111 and Firefox nightly 112 (my goal was only to have a sense of whether React v17 would bring by itself some improvements or regressions, so I didn't try to use similar versions).

I tested locally by running in the InteractiveRunner only the React workload React-TodoMVC and looked at the timings in a very unscientific way as I just skimmed through the results :-).

flashdesignory · 2023-03-14T15:20:07Z

Ah ok, and with those tests, which version was the fastest?

julienw · 2023-03-14T15:25:05Z

Ah ok, and with those tests, which version was the fastest?

Your new version, by a large margin!

I also profiled (both in chrome and Firefox) to see if anything was outside of the performance marks (that could explain the timings change); but no, everything is still inside the performance marks. So the app is probably just faster due to how it's built. I believe you're right that by memozing the Item component we're cutting out a lot of the work. Maybe it would be even more visible if we'd use more than 100 items -- maybe we should to better show how the browsers deal with bigger arrays (?) (but that would need to be discussed of course).

resources/todomvc/architecture-examples/react/src/todo/components/input.jsx

julienw · 2023-03-14T17:36:52Z

resources/todomvc/architecture-examples/react/webpack.common.js

+                        presets: [
+                            ["@babel/preset-env", { targets: "defaults" }],
+                            ["@babel/preset-react", { runtime: "automatic" }],
+                        ],


I see it's already in babel.config.js, is it necessary to have both?
Also it's not clear to me what environement babel targets here... It looks like it targets some recent version of JS but it would be good to explicit this, especially that babel seems to say otherwise...

thanks - I removed the babel.config file

:wq initial cleanup

flashdesignory · 2023-03-16T18:47:39Z

@julienw - please let me know where we are with this pr 😄

julienw · 2023-03-20T15:45:13Z

@julienw - please let me know where we are with this pr smile

sorry, I should have been more explicit: I thought this was ready to be merged indeed :)

gsathya · 2023-03-28T14:13:53Z

resources/todomvc/architecture-examples/react/package.json

+    },
+    "dependencies": {
+        "classnames": "^2.2.5",
+        "react": "^17.0.2",


Apologies for the drive-by question: but why pick v17 and not v18 (current stable)?

I believe there are 2 reasons:

v17 is still the most used version in the web currently.

v18 is much more asynchronous, so it's difficult (if possible) to determine when it finished rendering. We'll have to eventually solve this problem though...

so it's difficult (if possible) to determine when it finished rendering. We'll have to eventually solve this problem though...

At API Level, React ~v17 also has similar problem that is hard to know when the "rendering" finished and the 3rd argument of render() has been used for that.

After v18, these guide might be helpful

Replacing render with createRoot reactwg/react-18#5

https://react.dev/blog/2022/03/08/react-18-upgrade-guide

FYI: Some more context on why v17 was chosen is in this document that Thorsten put together.

There's a lot of difference between React v17 & v18 and I suspect the performance characteristics to be different too.

We expect v18 to be the most popular version soon (it's the version shipping with metaframeworks like Nextjs as well). According to our metrics, v18 is already the most downloaded version: https://rn-versions.github.io/

If we expect Speedometer to be future proof, then I'd strongly push to use React v18.

+1 to what @gsathya mentioned. NPM data suggests that 18 is already the most popular version, and in-the-wild numbers will naturally trail installs. It's really important that we're all optimizing for the right version and I'm very concerned about the choice of 17 here. There is a very big difference between 17 and 18, and optimizations that benefit one may not help the other.

If the question is about how to measure accurately in 18, we can help with that :-)

Thanks for the useful feedback, folks. I agree that v18 will likely have different performance characterstics compared to v17. It would make sense to include a version of the benchmark with v18.
But also please note that we also intend to update the benchmark more often than what we did so far.

flashdesignory requested review from bgrins and rniwa March 13, 2023 14:55

julienw reviewed Mar 13, 2023

View reviewed changes

rniwa approved these changes Mar 13, 2023

View reviewed changes

julienw reviewed Mar 14, 2023

View reviewed changes

resources/todomvc/architecture-examples/react/src/todo/components/input.jsx Outdated Show resolved Hide resolved

flashdesignory mentioned this pull request Mar 14, 2023

Benchmark Runner "Enter" vs "ENTER" #103

Closed

julienw reviewed Mar 14, 2023

View reviewed changes

flashdesignory force-pushed the todomvc-react-reducer branch from b9205f4 to e42ef11 Compare March 14, 2023 17:48

flashdesignory added 15 commits March 15, 2023 17:20

t:wq

426b4b0

:wq initial cleanup

cleanup html

0231fff

initial structure

6e267cd

more progress

73db20c

format react

71c72ce

install

8b4844f

add hooks

18e98c9

continue rebuild

cdc9d84

refactor

2cfaf98

eslint react

15caec4

make it editable

58110ff

make state work

fb99312

update model

a828bc9

cleanup

42efdcf

more cleanup

4e12f97

flashdesignory added 20 commits March 15, 2023 17:20

pr feedback part 2

d75d230

pr feedback part 3

ff73e8f

use reducer

9359b9f

memo item

546104e

pr feedback

e49674e

memo filter

3ccac10

rename files

3d9fb7b

lowercase filenames

59cb010

fix completed item

7d1d2c0

update build

12e03cc

format updates

6596067

update reducer

a5e813b

cleanup

0571e36

remove node_modules folder

ad3b949

pr feedback from preact

41376c9

adjust reducer

2cd27b5

cleanup

3ebd449

format

0950f5d

add ignore lines for prettier

ef5c9d1

remove whitespace

c5412b4

flashdesignory force-pushed the todomvc-react-reducer branch from 5d7a0bb to c5412b4 Compare March 16, 2023 00:20

flashdesignory added 2 commits March 16, 2023 11:41

Merge branch 'main' into todomvc-react-reducer

0eaf3b7

Enter

207c2c4

merge main

be74f31

bgrins approved these changes Mar 16, 2023

View reviewed changes

flashdesignory merged commit d755898 into WebKit:main Mar 16, 2023

flashdesignory deleted the todomvc-react-reducer branch March 16, 2023 22:21

gsathya reviewed Mar 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Todomvc react update #98

Todomvc react update #98

flashdesignory commented Mar 13, 2023 •

edited

julienw left a comment

julienw Mar 13, 2023

flashdesignory Mar 13, 2023

flashdesignory commented Mar 13, 2023

julienw Mar 14, 2023

julienw commented Mar 14, 2023 •

edited

flashdesignory commented Mar 14, 2023

julienw commented Mar 14, 2023 •

edited

flashdesignory commented Mar 14, 2023

julienw commented Mar 14, 2023

julienw Mar 14, 2023

flashdesignory Mar 14, 2023

flashdesignory commented Mar 16, 2023

julienw commented Mar 20, 2023

gsathya Mar 28, 2023

julienw Mar 28, 2023

tetsuharuohzeki Mar 28, 2023

housseindjirdeh Mar 28, 2023

gsathya Mar 30, 2023 •

edited

josephsavona Mar 30, 2023

julienw Mar 30, 2023

Todomvc react update #98

Todomvc react update #98

Conversation

flashdesignory commented Mar 13, 2023 • edited

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flashdesignory commented Mar 13, 2023

Choose a reason for hiding this comment

julienw commented Mar 14, 2023 • edited

flashdesignory commented Mar 14, 2023

julienw commented Mar 14, 2023 • edited

flashdesignory commented Mar 14, 2023

julienw commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flashdesignory commented Mar 16, 2023

julienw commented Mar 20, 2023

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

gsathya Mar 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flashdesignory commented Mar 13, 2023 •

edited

julienw commented Mar 14, 2023 •

edited

julienw commented Mar 14, 2023 •

edited

gsathya Mar 30, 2023 •

edited