-
Notifications
You must be signed in to change notification settings - Fork 71
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
Todomvc react update #98
Todomvc react update #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
||
return ( | ||
<form onSubmit={handleSubmit}> | ||
<input className="new-todo" id="todo-input" type="text" data-testid="text-input" autoFocus placeholder={placeholder} defaultValue={defaultValue} onBlur={handleBlur} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@julienw - I did and I am still comparing both the react and preact versions to determine what affects the scores. 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. |
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thevalue
- when the event is dispatched, react compares the current
value
with the saved one, they are the same value => as a result it swallows theinput
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.
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 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). |
@julienw - that's a great comparison. |
I compared on my machine (Xorg Linux Debian):
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 :-). |
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 |
resources/todomvc/architecture-examples/react/src/todo/components/input.jsx
Outdated
Show resolved
Hide resolved
presets: [ | ||
["@babel/preset-env", { targets: "defaults" }], | ||
["@babel/preset-react", { runtime: "automatic" }], | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - I removed the babel.config file
b9205f4
to
e42ef11
Compare
5d7a0bb
to
c5412b4
Compare
@julienw - please let me know where we are with this pr 😄 |
sorry, I should have been more explicit: I thought this was ready to be merged indeed :) |
}, | ||
"dependencies": { | ||
"classnames": "^2.2.5", | ||
"react": "^17.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the drive-by question: but why pick v17 and not v18 (current stable)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Some more context on why v17 was chosen is in this document that Thorsten put together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
numbers:
https://gist.github.com/flashdesignory/dfe0158371f7c6052dbfe4ba8767a96b
cc: @kara