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 svelte cleanup #324

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

flashdesignory
Copy link
Contributor

@flashdesignory flashdesignory commented Oct 4, 2023

Some cleanup items from this pr: tastejs/todomvc#2194
Since the changes are coming directly from the Svelte team, it would be beneficial to align.

three items I didn't carry over to Speedometer:

  1. uuid: I kept our implementation
  2. vite: I didn't touch rollup, which is used here

scores
chrome before:
chrome_before
chrome after:
chrome_after

safari before:
safari_before
safari after:
safari_after

firefox before:
firefox_before
firefox after:
firefox_after

edge before:
edge_before
edge after:
edge_after

@kara

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Interesting that these simple changes make the app so much faster. Looks like that using idiomatic structures helps svelte compile the app to more efficient code.

Should you regenerate the complex version of svelte too?

Small request: can you add

watch: {
  clearScreen: false,
}

to the rollup config
so that npm run dev keeps the server URL displayed?

Thanks :-)

@@ -11,7 +11,7 @@
"build": "rollup -c",
"watch": "rollup -c -w",
"serve": "http-server ./dist -p 7002 -c-1 --cors",
"dev": "npm run serve & npm run watch"
"dev": "npm run serve & npm run watch --no-watch.clearScreen"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julienw - let me know if this works for you

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this works but what is the advantage compared to adding it to the config?

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 threw an error for me 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

very weird because this worked for me:

diff --git a/resources/todomvc/architecture-examples/svelte/rollup.config.js b/resources/todomvc/architecture-examples/>
index b9bf6c67..4c1f9c26 100644
--- a/resources/todomvc/architecture-examples/svelte/rollup.config.js
+++ b/resources/todomvc/architecture-examples/svelte/rollup.config.js
@@ -12,16 +12,19 @@ const production = !process.env.ROLLUP_WATCH;
 export default {
     input: "src/index.js",
     output: {
         file: "dist/app.js",
         format: "iife",
         sourcemap: true,
         name: "app",
     },
+    watch: {
+        clearScreen: false,
+    },
     plugins: [
         css({
             minify: true,
         }),
         svelte({
             include: "src/**/*.svelte",
         }),
         resolve({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe it took a while for the coffee to kick in, but I moved it in the config and it works of course.

@@ -10,7 +10,6 @@
"devDependencies": {
"big-dom-generator": "file:../../big-dom-generator",
"http-server": "^14.1.1",
"jsdom": "^22.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpardosixtosMs - this change happened when I ran npm install, to be able to rebuild the new svelte version.

Copy link
Contributor

@issackjohn issackjohn Oct 5, 2023

Choose a reason for hiding this comment

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

That is okay. We've removed jsdom as a devDependency in package.json. I was using npm ci which doesn't update the lock files.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me think that we probably want npm install and not npm ci when updating the complex versions. Indeed we might run into problems when adding dependencies to the big-dom-generator. It would be good to double check.

@flashdesignory flashdesignory merged commit 32c7c23 into WebKit:main Oct 10, 2023
4 checks passed
@flashdesignory flashdesignory deleted the todomvc-svelte-update branch October 10, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants