Todomvc vue Step 01 #114

flashdesignory · 2023-03-20T15:51:50Z

This pr updates vue 2 -> vue 3. This step resulted in a lot of changed files, since I needed to install vue-cli 3.2.6 and then I manually updated vue to 3.2.47. With this "simple" upgrade, I needed to fix a few bugs that kept the app from building.
Additionally a lot of files from the previous build were consolidated by vue-cli.

The logic in the actual Todo app didn't change and a small refactor is still following with step 02.

Speedometer updates

renamed vue-cli to vue
added vue-webpack to .gitignore, .eslintignore, .prettierignore
renamed test suite and pointing to new directory

Vue updates

upgraded vue to vue@3.2.47
small folder reorg (this matches the new folder structure that vue creates if you start fresh)
added vue-router (this doesn't affect the tests since we don't navigate anywhere, but gives the app consistent behavior with the other apps)
added a key attribute to the todo-list (vue complaint)
fixed input field behavior
fixed toggle all button
removed local todo css styles in favor of the npm packages (to be consistent)

Scores:
https://gist.github.com/flashdesignory/6c9c30fdf68597d949e9fe3f61b90f8b

Since nothing changed in regards to adding, toggling or removing the todos (which the test performs), I assume the slight difference has to do with the vue update.

@kara

bgrins · 2023-03-20T20:12:57Z

resources/tests.mjs

@@ -248,8 +248,8 @@ Suites.push({
 });

 Suites.push({
-    name: "VueJS-TodoMVC",
-    url: "todomvc/architecture-examples/vuejs-cli/dist/index.html",
+    name: "TodoMVC-Vue-Webpack",


Why rename to include "webpack" (in the suite name but also the file structure)? My understanding from https://docs.google.com/document/d/1MXxk9DryJmvAmPMt2uMneaS3J5Y-dMFGt4EGvMzhhJE/edit#heading=h.175iv9aumkej is that we aren't, for example, planning to include multiple variations based on build tooling.

just personal naming preference - removing webpack from test name 👍

I'll also rename the directory shortly... uno momento!

bgrins

This seems reasonable to me, thanks. cc @julienw in case there's additional comments but I'm fine to land this

julienw · 2023-03-21T09:37:12Z

Looks reasonable to me as well!

julienw · 2023-03-21T10:28:21Z

All of the following comments can be handled follow-ups, and do not block this pull request.

I discussed with a friend who knows vue better than I do (@enguerran 👋), he was surprised by a few things:

binding syntax: we use both @ and v-bind in this code, we should probably decide which one we want to use and be consistent.
the use of nextTick for the focus: I believe it is superfluous. Indeed the directive function will be called on the updated and mounted phases, so the DOM is already uptodate at that point, and this would simplify the code a bit. I tried locally and it looks like the autofocus behavior still works for me.

I also tested the code locally. It works pretty well, but I found the following difference compared to the other versions:

it doesn't prevent adding a todo item with less than 3 characters. Especially it allows adding an empty item, which is breaking the layout.

flashdesignory · 2023-03-21T13:54:56Z

Thanks @julienw & @enguerran!
I'm planning on doing some clean-up once this is merged.

flashdesignory · 2023-03-21T19:10:06Z

@rniwa - how is this one looking?

rniwa · 2023-03-21T21:17:25Z

Deleting items sync got 3-6x slower in Safari, Firefox, & Edge but only 2x in Chrome. We need to understand that discrepancy.

flashdesignory · 2023-03-21T21:57:51Z

Deleting items sync got 3-6x slower in Safari, Firefox, & Edge but only 2x in Chrome. We need to understand that discrepancy.

The app logic didn't change and when you click the "x" button.
It goes through the same flow as before and there are no changes I can see that would result into a slower performance from the app side.

This leaves us with two areas:

Vue framework update from 2 -> 3
Vue webpack infra update (which pretty much comes optimized when you create a new vue app).

Additional changes were tested with each step and didn't introduce the score differences that you see:

removing the router (no change)
removing list key (no change)
using previous css styles
removing router links

The scores are hard to compare in my opinion when the base scores are already really small.
If a change adds for example 1 s no matter of the initial score, it looks like it has a lot larger impact than it actual does.
There is also some rounding going on in the scores, which might be unfavorable for these small numbers.

In this case it seems that "something under the hood" introduced the difference and it's hard to figure out exactly what that is. If it's the framework that somehow introduced the latency, should we not upgrade? If it's the infra (bundler, compression, optimization, transpiling) that comes with vue, is that something we should focus on and do look into differences (which essentially means we are comparing bundlers).

rniwa · 2023-03-21T22:06:06Z

The problem here is there is a massive discrepancy between browsers. I'm not comfortable accepting that without understanding why.

flashdesignory · 2023-03-21T22:15:18Z

The problem here is there is a massive discrepancy between browsers. I'm not comfortable accepting that without understanding why.

I can try separating the webpack update from the vue update and maybe that'll let us pinpoint where it's coming from.
Please note though that the webpack integration in this version is tightly coupled with creating the vue app (it's a one-step setup) and trying to change that setup comes with some challenges.

bgrins · 2023-03-21T22:46:09Z

Here's a comparison of a single iteration of the delete step:

Current (4.2ms): https://share.firefox.dev/3yVCQlh
New (9.6ms): https://share.firefox.dev/404QPBh

I haven't investigated it closely but it appears that the bulk of the difference is that the click handler the current one (2.1ms) has a single function e that is called and in the new one (6.9ms) there's a deeper stack through $bound -> deleteTodo -> filter+set which call a number of functions beneath them.

bgrins · 2023-03-21T23:06:53Z

To be clear, there are many click events in the delete step which are aggregated in that stack chart - they can be seen in the marker chart in DOMEvent for old: https://share.firefox.dev/3FInjct and new: https://share.firefox.dev/3JZmgrd. In the old version a few of the individual clicks are slow (but overall are fast enough that we may be missing samples underneath) but in the new version many of them are.

If this build setup is typical for Vue 3 I'm not too concerned about the discrepancy. @flashdesignory can you share what you have planned for the "small refactor is still following with step 02."? Maybe that will change how things are called a bit.

flashdesignory · 2023-03-22T01:28:05Z

@bgrins - thanks for looking into the differences! It's what I suspected, since the todo application didn't change from it's delete implementation.

<button class="destroy" @click.prevent="deleteTodo(todo)"></button>

deleteTodo(todo) {
     this.todos = this.todos.filter(t => t !== todo)
},

julienw · 2023-03-22T10:43:17Z

resources/todomvc/architecture-examples/vue/src/components/TodoComponent.vue

            <ul class="todo-list">
-                <li class="todo" v-for="todo in filteredTodos" :class="{completed : todo.completed, editing : todo === editing }">
+                <li class="todo" v-bind:key="todo.id" v-for="todo in filteredTodos"


The documentation about :key says that it should be just this, not v-bind:key => https://vuejs.org/api/built-in-special-attributes.html#key

BTW I see this wasn't here before. I doubt this would explain the performance difference but could it be worth looking?

yeah - :key is the shorthand I believe... will adjust
(also already tested, there is no performance gain when I remove it)

julienw · 2023-03-22T11:11:23Z

I had another look on profiles using the development server for each versions, so that I could see better what could be happening.
I first added 20 items, then while profiling I deleted 9 of them.

I noticed that in the new version we're calling renderComponentRoot that's itself calling a lot of things, while in the previous version we were calling render that's only calling renderList. We spend less time in that render function in the previous version that in renderComponentRoot in the new version.

See profiles (take them with a grain of salt because they're were taken while running the dev server):
Before upgrade: https://share.firefox.dev/3LI0lG5
After upgrade: https://share.firefox.dev/3LLeH8D

The list of things that renderComponentRoot calls makes sense, as they all have the todos list as a dependency. Therefore my guess is just as good as the previous guesses: vue 3 is slower than vue 2 when memoizing the computed properties.

Note: in case you want to try locally, please note that you'll need to use node v8 to run or build the old version.

julienw · 2023-03-22T11:12:34Z

Note: another obvious change compared to the old version is the addition of vue-router. Could it be a good idea to try to remove it in a hacky way just for trying?

julienw · 2023-03-22T13:33:32Z

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

+      "npm": ">=8.19.3"
+  },
+  "scripts": {
+    "serve": "vue-cli-service serve",


In the other components (I looked at react / react-redux / preact), we used start for the development server and serve for serving the result of the build. backbone uses dev which is more explicit.
On the long run it would be good to be consistent, but since serve hasn't been used yet, how about using either start or dev here?

yes, I am getting confused too. Ember-cli creates the scripts, but I'll rename to "dev"

flashdesignory · 2023-03-22T15:36:49Z

Note: another obvious change compared to the old version is the addition of vue-router. Could it be a good idea to try to remove it in a hacky way just for trying?

I did try that before, with no differences. I'll give it another try though.

update: tried again removing the router and no difference.

flashdesignory · 2023-03-22T20:54:00Z

@rniwa - with the added help from @bgrins and @julienw to look into performance between vue2 / vue3 - how would you like to proceed?

rniwa · 2023-03-22T21:32:03Z

I guess the question is if this change is intrinsic to Vue v2 vs. v3 or if it's something specific to the way it's written here.

bgrins · 2023-03-22T21:37:43Z

I believe it's intrinsic to 2 vs 3. For example from https://vuejs.org/guide/extras/reactivity-in-depth.html#how-reactivity-works-in-vue

There are two ways of intercepting property access in JavaScript: getter / setters and Proxies. Vue 2 used getter / setters exclusively due to browser support limitations. In Vue 3, Proxies are used for reactive objects and getter / setters are used for refs

bgrins · 2023-03-22T21:40:00Z

The Proxy thing being just one example. I think they work differently and so there are just going to be different things to optimize. With deleting in particular we can see no logic changes in the PR but we can see it's acting differently (and more slowly) now with 3.

flashdesignory · 2023-03-29T06:41:23Z

@rniwa - just following up on this pr - are we good to merge this one?

rniwa

@rniwa - just following up on this pr - are we good to merge this one?

Yeah, thanks for all your analysis, everyone.

flashdesignory added 15 commits March 17, 2023 14:35

vue initial

2417d13

html page

59eed2f

fix router

107e7d9

cleanup

905b397

use vue-cli

f51d51b

rename test

894bed9

config

24987c7

update config

b66257d

cleanup

29511d0

remove logo

f68951b

remove unused dirs

02f97a9

add vue

bad0baf

fix path

b158b3e

fix stuff

5b5a4b7

package.json

fadafad

flashdesignory requested review from rniwa and bgrins March 20, 2023 15:52

add dist dir

f913fe4

bgrins reviewed Mar 20, 2023

View reviewed changes

flashdesignory added 3 commits March 20, 2023 13:16

simplify name

405d4ba

rename workload directory

c6c5fa5

remove node_modules

cfcfebc

bgrins approved these changes Mar 20, 2023

View reviewed changes

julienw reviewed Mar 22, 2023

View reviewed changes

pr feedback

fa02ac4

flashdesignory added 4 commits March 23, 2023 08:46

merge main

81b738f

install vue eslint

1d292c3

opt vue out of format for now

06e523c

merge main

a967cce

flashdesignory added 4 commits April 3, 2023 13:36

merge conflict

6517a8c

reset package-lock

08b8ec0

Merge branch 'main' into todomvc-vue-01

5e22500

eslint vue

f612228

bgrins mentioned this pull request Apr 7, 2023

Todomvc angular 01 #129

Merged

rniwa approved these changes Apr 7, 2023

View reviewed changes

flashdesignory merged commit 05b6f7a into WebKit:main Apr 7, 2023
4 checks passed

flashdesignory deleted the todomvc-vue-01 branch April 7, 2023 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Todomvc vue Step 01 #114

Todomvc vue Step 01 #114

flashdesignory commented Mar 20, 2023 •

edited

bgrins Mar 20, 2023

flashdesignory Mar 20, 2023

flashdesignory Mar 20, 2023

flashdesignory Mar 20, 2023

bgrins left a comment

julienw commented Mar 21, 2023

julienw commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

rniwa commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

rniwa commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

bgrins commented Mar 21, 2023

bgrins commented Mar 21, 2023

flashdesignory commented Mar 22, 2023

julienw Mar 22, 2023

flashdesignory Mar 22, 2023

flashdesignory Mar 22, 2023

julienw commented Mar 22, 2023 •

edited

julienw commented Mar 22, 2023

julienw Mar 22, 2023

flashdesignory Mar 22, 2023

flashdesignory Mar 22, 2023

flashdesignory commented Mar 22, 2023 •

edited

flashdesignory commented Mar 22, 2023

rniwa commented Mar 22, 2023 •

edited

bgrins commented Mar 22, 2023

bgrins commented Mar 22, 2023

flashdesignory commented Mar 29, 2023

rniwa left a comment

Todomvc vue Step 01 #114

Todomvc vue Step 01 #114

Conversation

flashdesignory commented Mar 20, 2023 • edited

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

bgrins left a comment

Choose a reason for hiding this comment

julienw commented Mar 21, 2023

julienw commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

rniwa commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

rniwa commented Mar 21, 2023

flashdesignory commented Mar 21, 2023

bgrins commented Mar 21, 2023

bgrins commented Mar 21, 2023

flashdesignory commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Mar 22, 2023 • edited

julienw commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flashdesignory commented Mar 22, 2023 • edited

flashdesignory commented Mar 22, 2023

rniwa commented Mar 22, 2023 • edited

bgrins commented Mar 22, 2023

bgrins commented Mar 22, 2023

flashdesignory commented Mar 29, 2023

rniwa left a comment

Choose a reason for hiding this comment

flashdesignory commented Mar 20, 2023 •

edited

julienw commented Mar 22, 2023 •

edited

flashdesignory commented Mar 22, 2023 •

edited

rniwa commented Mar 22, 2023 •

edited