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

Rewrite #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rewrite #5

wants to merge 1 commit into from

Conversation

sirenkovladd
Copy link
Contributor

Same pages
Same design
Same DOM

  • Added client router (change page without reload)
  • better SSR
  • added minification
  • base on node
  • grouped by folders
  • used step-by-step build with wireit
  • removed build files

Side effect

  • Several page headers have had their id changed (changed links to several pages pointing to the header). Cause: JSDOM does not support innerText

For test

npm i
npm run preview

You can run with hot reload

npm run preview --watch

Also I added file compare-dom.js for check difference in DOM

node compare-dom.js https://vanjs.org http://127.0.0.1:8080

You should first format the html in diff/ (I used built-in tools in vscode) and then use any tools to compare the same pages (vscode also does this)

@Tao-VanJS
Copy link
Member

Thank you so much for the contribution!

However, I feel the PR is too large to review and ensure it doesn't break anything. I looked at the summary, it seems among all the things, only client-side routing can result noticeable improvement on user experience. Even for that, I believe it's not significant given that in the current implementation, the subsequent page load is expected to be satisfactorily fast as most of the assets will hit cache.

@sirenkovladd
Copy link
Contributor Author

Yes, I agree, it is difficult to check

that's why I added a script that will help check the result of the pages
also now I see that I have access to all files from the repository such as https://vanjs.org/sitegen/node-examples/van-plate-server/package.json
now the project has quite a complex folder structure
I tried to group it so that it would be more convenient to make changes in the future
Compressed files allow you to load the page faster and take up less space in the cache

If you check that the result is exactly the same as it is now
after that I see no reason not to merge it
as the repository will be more reliable, clearer and faster

@Tao-VanJS
Copy link
Member

Thanks so much! I understand it's really lots of work behind this PR.

But I'm not quite convinced on the merging. There are a few sticking points:

Several page headers have had their id changed

This is not desirable. This will break the links pointing to various sections of vanjs.org). Also, if the directory structure is changed, it will break existing links to jsfiddle and CodeSandbox as well.

now the project has quite a complex folder structure

Not sure if this is the problem that needs to be solved. If we look at the sitegen folder, it doesn't look complex to me. Every individual page has a corresponding .ts file. It's easy to find the right file to work on for each page. I don't think the current situation makes it hard for people to work on.

Compressed files allow you to load the page faster and take up less space in the cache

Is there any benchmark to quantify how much gain we can expect? I am interested to see the impact to real-world users. (Let's say we deploy the new implementation via GitHub pages and compare the real-world performance against vanjs.org). The change of compression and bundling might affect how cache behaves thus it's really hard to tell the actual impact of performance without the actual benchmark data.

base on node

I am not sure whether the move from Deno to Node is a good direction. Deno is the newer runtime with more modern features and is easier to use. And deno-dom has better performance than jsdom: https://docs.deno.com/runtime/manual/advanced/jsx_dom/deno_dom.

removed build files

Also I believe having the result HTML files in the repo is a feature rather than a bug. It's quite desirable to have the ability to see the actual diff in HTML files caused by each change in .ts files under sitegen.

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.

2 participants