PostMessage Cleanup - TodoMVC example (experimental) #509
Conversation
✅ Deploy Preview for webkit-speedometer-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
flashdesignory
changed the title
| *************************************************************************/ | ||
| export class BenchmarkConnector { | ||
| constructor(suites, name, version) { | ||
| this.suites = suites; |
There was a problem hiding this comment.
Is there a benefit in refactoring? Maybe that's a larger conversation if we want to prefer private variables in our code and might be better to tackle in a separate discussion. This is consistent with the previous example that uses the postMessage api and I rather want to avoid larger changes in a single pr.
| * | ||
| * A single test step, with a common interface to interact with. | ||
| */ | ||
| export class BenchmarkStep { |
There was a problem hiding this comment.
It's not great that we're duplicating all this code.
We should figure out a way to share code.
There was a problem hiding this comment.
these files come from the ./resources/shared and are used in the workload by installing the local 'speedometer-utils' package. This links the files and keeps them in sync. This has been discussed and agreed on when I added the news site example that uses postMessage.
| @@ -0,0 +1,195 @@ | |||
| export class Params { | |||
There was a problem hiding this comment.
We shouldn't duplicate all this code. It would be a maintenance nightmare.
There was a problem hiding this comment.
see above comment :-)
| @@ -0,0 +1,86 @@ | |||
| class TestInvoker { | |||
There was a problem hiding this comment.
We should figure out a way to share code here as well.
There was a problem hiding this comment.
see above comment :-)
resources/todomvc/vanilla-examples/javascript-web-components/scripts/build.js
Outdated
Show resolved
Hide resolved
resources/todomvc/vanilla-examples/javascript-web-components/scripts/build.js
Outdated
Show resolved
Hide resolved
resources/todomvc/vanilla-examples/javascript-web-components/scripts/build.js
Outdated
Show resolved
Hide resolved
| * @param {string} dest Destination directory. | ||
| */ | ||
| async function copyDirectory(src, dest) { | ||
| await fs.cp(src, dest, { recursive: true }, (err) => { |
There was a problem hiding this comment.
note that cp was experimental until node v22.
I don't mind but it would be good to make sure everybody is OK with that and be explicit about us supporting node v22+.
There was a problem hiding this comment.
Ah yeah... that should be done in a follow up, since a few workloads use a similar script.
The workloads themselves typically pin a node version, so it's not super critical to fix right away.
resources/todomvc/vanilla-examples/javascript-web-components/scripts/build.js
Outdated
Show resolved
Hide resolved
| if (err) | ||
| console.error(err); | ||
| }); | ||
| await copyDirectory("./src", "./dist/src"); |
There was a problem hiding this comment.
I'm not sure why you're not copying to /dist/ simply?
It looks like this would give less changes overall...
There was a problem hiding this comment.
I wanted to avoid to dump everything in the root of the dist folder.
…cripts/build.js Co-authored-by: Julien Wajsberg <[email protected]>
…cripts/build.js Co-authored-by: Julien Wajsberg <[email protected]>
…cripts/build.js Co-authored-by: Julien Wajsberg <[email protected]>
This pr uses the todomvc web components workload and enables it for the post message api.
This is similar to what we've done for the news site example.
Benchmark changes
Workload changes
chrome:
safari:
todo: cb scores