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

TodoMVC workload based on React 18 / React Router / Material UI #373

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Feb 15, 2024

Hi,
I'd like to propose this new workload in tentative/.
It's based on React 18, React Router and Material UI.

Links:

I decided to use Material UI as a way to get deeper React trees.
I also used React Router because it's very much used. The way I used it to control the model might lead to discussions though: I thought of it as a way to "emulate" a server side environment. I believe that if we'd have to implement a TodoMVC app with a server side environment, we could use the same architecture. Here of course I used a local model, not a server.
Because of React 18 (and I believe the CSS-in-JS library used in Material UI) I had to override rAF and setImmediate so that the SP3 runner could capture all the work.
Especially I had to use microtasks (with promises), not normal tasks (with setTimeout) like before. Indeed with just setTimeout, it was happening that we'd get a setTimeout called in another setTimeout, and this was sometimes escaping the benchmark runner.

I also decided to use some more steps in the benchmarks, than in the other TodoMVC workloads. I added:

  • move between "active" and "all" items
  • clear completedi tems

For review, I split this in several commits to make it easier to review, but the bulk of the implementation is in Implement TodoMVC with all its actions.

If we agree on the general approach, but there are small changes to be done, I'd rather land this first and implement the changes in follow-ups. Hopefully this makes sense!

Happy to hear feedback, folks

new BenchmarkTestStep("CompletingSomeItems", (page) => {
const checkboxes = page.querySelectorAll("input[type='checkbox']");

// We'll be checking 5 checkboxes, in different parts of the list.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's 20, not 5

Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to update the comment? 🤔 😄

children: [
{
path: "new",
action: async ({ request }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I like the way the router is set up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
Yeah, it's the new way introduced in react-router v6.4, I like it as well!

@camillobruni
Copy link
Contributor

Thanks, looking nice! ...and sorry for the delay.

Q1: Do think it would make sense to display the summary footer as well in the speedometer view?
Screenshot 2024-06-05 at 12 06 40

Q2: I do see a fair amount of unmeasured style and layout updates in chrome.
a) Could you confirm this is the same in firefox?
b) Probably harder: Could we somehow fore measure this as well?
Screenshot 2024-06-05 at 12 08 11

Q3: Would it be interesting to also add a step scroll/jump to an item in the middle of the list?

@camillobruni
Copy link
Contributor

camillobruni commented Jun 5, 2024

Both Chrome and Firefox seem to have an ok score, Safari Version 17.5 (19618.2.12.11.6) and Release 195 (Safari 17.4, WebKit 19619.1.15.0.1) both seem to struggle massively in the delete step (100x slower than FF).
@rniwa is this something you might want to have a look at?

Screenshot 2024-06-05 at 12 15 19

Compare to Chrome Canary 127.0.6519.0 :
Screenshot 2024-06-05 at 12 16 46

And Firefox Nightly 127.0a1 (2024-05-12) (64-Bit):
Screenshot 2024-06-05 at 12 17 22

import type { Params } from "react-router-dom";
import { getTodos } from "./todo-model";

export function loader({ params }: { params: Params<"filter"> }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

super duper nit: the name loader threw me off... would it be possible to rename it to something like:
getTodos or getFilteredTodos ?

Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

Overall this looks like a nice addition.
nit: lint step fails at the moment.

@bgrins
Copy link
Contributor

bgrins commented Sep 9, 2024

@rniwa we discussed getting this one landed in an experimental folder. Care to review before we do that?

@bgrins bgrins requested a review from rniwa September 9, 2024 20:12
Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

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

seems okay to add this experimentally.

@@ -304,6 +304,50 @@ Suites.push({
],
});

Suites.push({
name: "TodoMVC-React-18",
Copy link
Member

Choose a reason for hiding this comment

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

Can we disable this workload by default since it's experimental?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants