Complex DOM versions of todoMVCs

[issackjohn]

The source components were moved without any modifications. A minor change was made to the Vue structure to match the expected structure of generated.css.

This pull request includes the following changes:

• Complex DOM versions now use a buildComplex.js script from the big-dom-generator package to build their distributions.
• The script works by taking the output files from the standalone version's dist folder and making any necessary changes, including copying over two CSS files and one logo.png. One CSS file is for the styling of the surrounding complex app, while the other is generated.css, which contains matching and non-matching CSS rules.
• It updates the dist folder path for the react complex version in test suites.
• It moves complex-static-html out of tentative and renames to it big-dom-generator.
  • removed react todomvc that was in this folder.
• It adds big-dom-generator as a dependency to complex versions using npm link big-dom-generator.

Affected:

• angular
• backbone
• jquery
• preact
• react
• react-redux
• svelte
• vue
• javascript-es5
• javascript-es6
• javascript-es6-webpack

There will be a follow up PR which adds the test suites for these Complex DOM versions.

- complex, shared, standalone folders for each TodoMVC in our affected list.
angular has projects/standalone, projects/complex, projects/shared-library

• It allows TodoMVCs to reuse components between standalone and complex versions.
• some index.html files were separated into index.html and partial.html so both standalone & complex can make use of the same files when building.
  • complex builds will take the body of big-dom-generator/dist/index.html, inject partial.html into it as

  .replace('<div class="todo-area">', `<div class="todo-area"><div class="todoholder">${partial.html}</div>`)

and then inject this resulting string into the body of index.html.

[issackjohn]

@lpardosixtosMs I'm not able to add you as a reviewer so could you please join when you get the chance?

[rniwa]

I don't think this is a "major change" given that it's not gonna affect the score. The PR which modifies tests.mjs to include complex DOM should be labeled a major change instead.

Chrome

• Before

  • 48.9 ± 1.3
  • 49.3 ± 1.4
  • 48.3 ± 1.4

• After

  • 49.1 ± 0.65
  • 48.6 ± 1.6
  • 49.6 ± 1.0

Firefox

• Before

  • 37.7 ± 1.3
  • 37.8 ± 1.2
  • 37.6 ± 1.3

• After

  • 37.2 ± 1.3
  • 37.8 ± 1.1
  • 37.9 ± 1.2

Safari

• Before

  • 40.2 ± 0.49
  • 40.9 ± 1.4
  • 40.3 ± 0.69

• After

  • 41.8 ± 0.96
  • 41.2 ± 1.1
  • 40.7 ± 0.87

complex-dom.zip

[julienw]

Thanks for this huge work, I believe this will be a very good addition to the benchmark.

I left a few comments for some problems I noticed, or just improvements, tell me what you think!

The only workload I don't understand fully is Angular, I'm not familiar with it and it's very different to the other frameworks. Would you please explain this a bit more? Why is it constructed so differently than the others?

Thanks!

[issackjohn]

Thanks for this huge work, I believe this will be a very good addition to the benchmark.

I left a few comments for some problems I noticed, or just improvements, tell me what you think!

The only workload I don't understand fully is Angular, I'm not familiar with it and it's very different to the other frameworks. Would you please explain this a bit more? Why is it constructed so differently than the others?

Thanks!

Thank you for taking the time to review!

I am also not familiar with Angular, but I'll do my best to explain my understanding of it.

A project is a set of files that comprise a standalone application or a shareable library. In the other projects I separated out shared components between the complex and standalone versions to a shared folder. For angular though, I went with this structure.

There is another way to do this, with a shared module. I went with the library approach, but I'm not 100% sure that's the best solution. IIUC, the only difference is that a library can be distributed as an npm package. If someone with more Angular background could weigh in that would be helpful and I'm happy to make the required changes.

structure of output

The default output of angular applications with multiple projects in them is:

dist /
  project-one/
  project-two/
  shared-library/

If we like this structure better, we can change it to that instead of

projects/
  project-one/
    dist/
  project/-two/
    dist/
  shared-library/
    dist/

[rniwa]

What's the next step for this PR?

[flashdesignory]

What's the next step for this PR?

I think once the requested changes from @julienw are done, we can review again

[issackjohn]

What's the next step for this PR?

I think once the requested changes from @julienw are done, we can review again

Yes, I'm currently working on the requested changes.

[flashdesignory]

Why are we changing the directory structure of the standalone versions?
My understanding was that the css changes that were previously discussed is all that's needed. We should NOT alter the directory structure and split files into complex / shared / standalone. Whatever the complex wrapper is doing needs to be independent, outside of the original todomvc apps.

It was my understanding that the standalone apps generate output files in the dist folder that can be consumed by the complex wrapper app.

[flashdesignory]

Here is an example that's acceptable for the folder structure:
Everything in the standalone version should remain the same as it is now in "main" after the css updates that were already merged.

[lpardosixtosMs]

Why are we changing the directory structure of the standalone versions? My understanding was that the css changes that were previously discussed is all that's needed. We should NOT alter the directory structure and split files into complex / shared / standalone. Whatever the complex wrapper is doing needs to be independent, outside of the original todomvc apps.

It was my understanding that the standalone apps generate output files in the dist folder that can be consumed by the complex wrapper app.

@flashdesignory I'm not sure how to do this just by working over the standalone dist folder, I can imagine it working for simple bundles like react/javascript-es5 but I'll let @issackjohn comment on the others if needed. It also seems unrealistic to add and modify the already bundled files. What we can do is go back to the old folder structure and let the complex version temporarily copy or point to the standalone sources and build the whole app just like it is doing right now, WDYT?

[flashdesignory]

@issackjohn - thanks for making all these changes. I know that was a lot of work.
I took a quick look and overall it looks a lot cleaner and simpler and I like the idea of the local installs for the todomvc apps 🎉 .

I'll review on Monday though since there's a lot to go through!

[flashdesignory]

This looks good to me - a huge change from before.
let's wait for @julienw to review as well!

[flashdesignory]

nit: that's something we didn't think about when naming the todomvc standalone apps.
Looks weird now when installing the local todo app - what do you all think? should we rename all the standalone versions to be explicit?

[issackjohn]

+1

[flashdesignory]

it could be something like:

todomvc-angular
todomvc-angular-complex

todomvc-es5
todomvc-es5-complex

what do you think @julienw @issackjohn ?

[issackjohn]

SGTM. There aren't plans to publish these packages to npm right? I see todomvc-react already published there.

[flashdesignory]

no plans to publish to npm.

[julienw]

That said, I wonder why we have to install them instead of refering to the source files directly in build.js. It seems to me that this is an unneeded indirection. What do you think? Is there a reason I'm not seeing?

[julienw]

Also the build scripts rely on the fact that the standalone versions have been built with their latest source. I'm not sure we should assume this, and instead we could run npm i && npm run build on the related standalone version first.

[flashdesignory]

using a local install vs just pointing to the local folder doesn't bother me. I think that's consistent with the way we use the todo-css and news-css folders currently.

Regarding running the build script before, I think that's an option to force a rebuild before... there are probably reasons for either way. It would be nice if we'd be using pnpm, cause then we could install a local package by version 🤷

[julienw]

It makes sense for the other cases because we're using import or require on them (as far as I remember), but here we're merely copying the files.

[issackjohn]

it could be something like:

todomvc-angular
todomvc-angular-complex

todomvc-es5
todomvc-es5-complex

what do you think @julienw @issackjohn ?

sounds good to me

Done.

[issackjohn]

@issackjohn - thanks for making all these changes. I know that was a lot of work. I took a quick look and overall it looks a lot cleaner and simpler and I like the idea of the local installs for the todomvc apps 🎉 .

I'll review on Monday though since there's a lot to go through!

@flashdesignory Thank you for your review! There was a lot to go through & I appreciate you taking the time. 🎉

[julienw]

Sorry I've put individual comments instead of just one review. This looks much simpler than before. I left a few comments, tell me what you think.

[issackjohn]

Sorry I've put individual comments instead of just one review. This looks much simpler than before. I left a few comments, tell me what you think.

I appreciate you taking the time to review! Thank you.

[issackjohn]

@julienw Are there any outstanding requested changes?

[julienw]

Please remove javascript-es6-complex and the changes to javascript-es6 as they will be removed in #283, so that there won't be any conflict.

[julienw]

Thanks, I believe this can go in like this.

For follow-ups (I'm serious: not for this patch!):
My main concern is that just running npm run build in the "complex" directories doesn't work without doing some prerequisites:

1. npm i in big-dom-generator
2. npm run build in the standalone directory

I think these dependencies should be run or checked when running the build scripts, or part of npm run build.
It's not clear to me why adding jsdom in the dependencies list (in big-dom-generator) doesn't install it when running npm i in the complex directories, but that may be how filepath-based dependencies work (I'm not too familiar with that).

I think we'll have to fix this issue, but, let me repeat myself, not in this PR :-)

Thanks!

[issackjohn]

Thanks, I believe this can go in like this.

For follow-ups (I'm serious: not for this patch!): My main concern is that just running npm run build in the "complex" directories doesn't work without doing some prerequisites:

1. npm i in big-dom-generator
2. npm run build in the standalone directory

I think these dependencies should be run or checked when running the build scripts, or part of npm run build. It's not clear to me why adding jsdom in the dependencies list (in big-dom-generator) doesn't install it when running npm i in the complex directories, but that may be how filepath-based dependencies work (I'm not too familiar with that).

I think we'll have to fix this issue, but, let me repeat myself, not in this PR :-)

Thanks!

Thank you again for your review! This all sounds good to me. I will submit a follow-up PR with the revisions once we land the JavaScript-web-components & Lit complex DOM versions, which are critical for the code freeze.