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

Parcel frontend starts from html instead of Vue #1889

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chengr4
Copy link
Contributor

@chengr4 chengr4 commented Apr 8, 2024

Hi,

I currently facing a crazy error. When I ran npm run watch with commit 61aeb07, I got the error.

@parcel/namer-default: Bundle group cannot have more than
one entry bundle of the same type

It seems that the root cause is because the shared site/frontend/src/components/tab.vue is imported into two places. Has anyone faced any similar issue before? (Interestingly, as-of.vue worked fine).

The discussion I found was here and people suggested to move parcel to vite 😅


References

@Kobzol
Copy link
Contributor

Kobzol commented Apr 8, 2024

Hi, I was afraid that something like this could happen. Sadly, Parcel is quite buggy in terms of multi-page builds, and I had to jump through a lot of hoops to get it working last year. I thought that reusing the same component in multiple top-level pages should work, but maybe it doesn't.

I have a vacation this week, so I won't be able to look at this until ~next week, or maybe even a bit later. Maybe we will be forced eventually to move to a different build system, I have evaluated a few last year, including vite, but sadly most of them are prepared for SPAs, but not very well for the setup that we use for rustc-perf (where we have N top-level pages built separately). Maybe just writing a few-line manual webpack config would be the easiest way to resolve this.. I don't know :)

@chengr4
Copy link
Contributor Author

chengr4 commented Apr 8, 2024

@Kobzol
Then I will slowly search for a solution until you are back and make an estimation.
I also feel as-of works, but tab does not work makes no sense.

No hurry. Have a nice vacation :)

@mischnic
Copy link

mischnic commented Apr 12, 2024

(Concrete suggestion for parcel-bundler/parcel#6310 (comment))

AFAICT, you don't actually use any dynamic SSR templating, so running Parcel on the templates (together with some posthtml plugin like https://github.com/posthtml/posthtml-extend) should be possible and resolve that error because now the shared CSS bundles can be handled (= included in the HTML) just fine.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 12, 2024

I have tried this before and ran into some issues that I sadly don't remember anymore. I guess that we can try again.

@chengr4
Copy link
Contributor Author

chengr4 commented Apr 15, 2024

@mischnic Thanks for the concrete suggestion. I will try it and wish us all the best 🥹.

@Kobzol If we change the entry point to template, I believe that how the backend imports the frontend will also be influenced. I will start tracing the code for this, and if you can guide me to the related place, it will be a great help


Update 1: I think is here site/src/resources.rs

@Kobzol
Copy link
Contributor

Kobzol commented Apr 15, 2024

Yeah, that file basically embeds the HTML templates from disk into the site binary (in release) or loads them from disk (in debug).

@chengr4 chengr4 changed the title Add Tabs to graphs page Parcel frontend starts from html instead of Vue Apr 27, 2024
@chengr4
Copy link
Contributor Author

chengr4 commented Apr 27, 2024

It worked at my local, feel free to try it.

What I have done

  • Move all html template up to the same folder of layout.html
  • html template imports uPlot directly from node_module (because it will be bundled together later)
  • Change bundle setting in package.json
  • Fix some paths to allow the bundle to pass through (e.g. <link rel="alternate icon" type="image/png" href="../../static/favicon.svg"> in layout.html)
  • Fit the frontend import path at site backend
  • add some unit test

Q/A

Q: Why I move all html template up to the same folder of layout.html?
A:
we hope to keep this block in layout.html that affects URL as the same.

<div>&gt; <a href="graphs.html">graphs</a>, <a href="compare.html">compare</a>,
    <a href="dashboard.html">dashboard</a>, <a href="bootstrap.html">bootstrap</a>,
    <a href="status.html">status</a>, <a href="help.html">help</a>.
  </div>

If we put other templates in the pages folder, it must be changed to e.g. <a href="pages/dashboard.html">dashboard</a>

What I am concerned

Because we change the path to import frontend, I believe some code need cleaning up. E.g.

// Maybe to clean up I am not sure
#[derive(RustEmbed)]
#[folder = "frontend/dist"]
#[include = "*.js"]
#[include = "*.br"]
#[include = "*.css"]
struct StaticCompiledAssets;

Therefore, It takes extra time to clarify when scenarios e.g.StaticAssets, StaticCompiledAssets, TemplateAssets are called, and it's best to refactor them with test support.


(This PR is a breaking change, so scaring...)


Update 1:

Unit test does not pass CI. It seems that I need to create a mock HTML. If there is any better way, please let me know. Thanks!

Update 2:

I installed Node.js and built the frontend in the CI pipeline for unit testing. I hope it is allowed.

@chengr4 chengr4 force-pushed the add-graphs-tabs branch 5 times, most recently from 72eec35 to 332c8ac Compare April 28, 2024 02:11
@chengr4 chengr4 marked this pull request as ready for review April 28, 2024 02:29
@Kobzol
Copy link
Contributor

Kobzol commented Apr 28, 2024

Thanks for looking into this! I'm a bit busy at the moment, I will try to take a look during the next week, but it's possible that I'll only have time after https://2024.rustnl.org/.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 10, 2024

Hi, sorry for the delay. I think that I start remembering now what were the issues with this approach 😆 It makes go Parcel go crazy.

If you check out e.g. the graphs page after this change, it includes about 5 JavaScript files (!), including the JS for the bootstrap page, the dashboard, etc. Or so it would seem - actually, these files probably include the right JavaScript for the graphs page (and nothing more), but the naming of these files is totally off (and also, it should ideally be in a single file, not 5 files). The chunking behaves in a very weird way here. I think that this happens because Parcel assumes that there will be only one top-level HTML page.

I tried to remove all the toplevel HTML files and keep just one (graphs), but even then it behaves in a weird way, and creates two copies of the ~same JavaScript file:
image

I suppose that this is somehow code splitting happening in practice. I tried to disable code splitting with:

"@parcel/bundler-default": {
    "minBundles": 999,
    "minBundleSize": 9999999,
    "maxParallelRequests": 20
  },

and it seems to help. With this, there are still two files per page, but it's just a module/nomodule split, and the JS files seem to contain all the requires JavaScript for that page. We could introduce some manual bundles (e.g. for Vue), but for now I'd like to keep all JS per page in a single file, if possible.

But sadly, the same thing happens for CSS. When I shared CSS of a component across multiple pages, the graphs page imports a bootstrap.css file... that's super confusing.

Now, it's possible that this would work better if our HTML pages were actually built using Parcel. Now they are just copied to dist and the layouting is performed dynamically. layout.html shouldn't be an additional page, instead the template should just extend this layout during build. But this currently does not happen. I think that we should use a Parcel PostHTML plugin, or something like that, to actually transform the HTML during the Parcel build.

So, yeah, this kind-of works, but the multiple weird named JS/CSS files are what put me off last time I tried this.

Do you want to try to experiment with the PostHTML plugin?

@chengr4
Copy link
Contributor Author

chengr4 commented Jun 15, 2024

@Kobzol
Regarding:

But sadly, the same thing happens for CSS. When I shared CSS of a component across multiple pages, the graphs page imports a bootstrap.css file... that's super confusing.

I assume you were talking about this,

2024-06-15_21-41-37.mp4

I believe the issue comes from both templates/graphs.html and templates/bootstrap.html importing node_modules/uplot/dist/uPlot.min.css.

When Parcel bundles CSS, it encounters node_modules/uplot/dist/uPlot.min.css in templates/bootstrap.html first, leading to its naming as bootstrap.4cf79458.css.


If I understand correctly, for HTML, what we want is for layout.html to be embedded within other HTML files, and we shouldn't see it in the dist directory no more.

I am very willing to try PostHTML to solve this.

But what about multiple JS chunk files? you mean that you find it acceptable even though you don't like it 🤣?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 15, 2024

But what about multiple JS chunk files? you mean that you find it acceptable even though you don't like it 🤣?

Yeah, Parcel is combining all shared JavaScript into multiple bundles, I'd like to avoid that, if possible. It looks like the configuration with minBundles that I sent here does prevent it, but I don't know how to configure the same thing for CSS. For JS it should be solved by the config I sent before.

f I understand correctly, for HTML, what we want is for layout.html to be embedded within other HTML files, and we shouldn't see it in the dist directory no more.

Yes, exactly :)

@chengr4
Copy link
Contributor Author

chengr4 commented Jul 26, 2024

@Kobzol
First, I apologize for taking a month to start thinking about the problem you gave me: extending layout.html.
I understand that asking you to revisit this PR to refresh your memory might be tiring 🙇‍♂️.

To sum up, I'm beginning to doubt whether this is the right thing to do.
The core reason is that we use the tera engine on the backend to parse tera templates for frontend rendering.
From this perspective, if we embed layout.html into other HTML files, it loses the value of using tera.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 26, 2024

I actually added tera relatively recently, and I don't think that we use it for actually passing any data from the backend to the frontend. The only reason I used it was to share some common layout code (e.g. the top menu and the footer) between all HTML pages. Sharing of this can easily be done also with frontend templates, so from this point of view it should be fine and we could even get rid of Tera in that case, I suppose.

So if using Parcel HTML layout templates allows us using Vue components across pages, and doesn't produce these weird JS/CSS artifact issues, then I would be fine with it.

So that we do not need to bundle `layout.html` using parcel

	ref:
	* https://parceljs.org/languages/html/#posthtml
@chengr4
Copy link
Contributor Author

chengr4 commented Jul 27, 2024

@Kobzol I rewrote templates with posthtml-include to embed layout.html.
Please take your time to slowly test this. Feel free to share your ideas with me and don't hesitate to ask any questions you need. I wish I am on the right way.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 31, 2024

Thank you. I'm currently still finishing my PhD thesis and I don't want to get into the weeds of more complicated stuff at the moment. I will try to take a proper look at this again in (mid-)August.

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.

3 participants