Open elite174 opened 5 months ago
@elite174 Is the order from ssr alone correct (that is if you disable js in your browser)?
@katywings yes, there're styles only in head, but nothing injected into body
but when I defer stream on the server the styles are injected into the body already, so even with disabled js the order is wrong and the styles are duplicated
@elite174 Could you make a repository with a reproduction or a Stackblitz? I just tried to reproduce this for myself and failed, no duplicated styles in head and no incorrect order 😅. Thank you in advance!
@katywings ok
Trying to reproduce... Noticed one detail: in stackblitz all styles are injected into head with rel="preload"
in my repo the styles are injected into head with rel=stylesheet
Will continue trying to reproduce
@katywings Wow, I reproduced it... Well, I'll cleanup my example and post the link here. The issue is REALLY weird, I'm even not sure that it's vinxi's fault... But we need to investigate it here.
https://stackblitz.com/edit/github-amsqsj?file=src%2Froutes%2F(page-layout)%2F(home).tsx
This is the link.
The issue is the following. Let's imagine we have page layout, a page and a component. The structure is the following:
Now let's imagine that page layout creates a "UI context" for children elements, so it can render children either as fullwidth
(items don't have inline margin) or as regular
(which means items have some inline margin in this case). How to distinguish these items inside layout? We may create CSS classes like:
.layout {
& > .regular {...}
& > .fullwidth {...}
}
How items will know what classes to use? We may export them from (page-layout).tsx
:
export const PAGE_LAYOUT_CLASSES = {
regular: styles.regular,
fullwidth: styles.fullwidth
}
Now any child of PageLayout can import these classes and use them to position within the layout correctly. The problem is with this classes import: when we import these classes, stylesheets are duplicated.
You may play with the example.
Check (home).tsx
- line 4 is the problem.
Check PageLayout.tsx
- there we export classes.
The question is: is a vinxi problem or is it a problem at all? Is it intended behaviour (style duplication) when we import classnames inside another file? Seems like there shouldn't be any duplication because we export map of strings, but I imagine that the bundler goes DFS and resolves styles at the deepest level first...
I guess it won't be a problem if there was some kind of "prebuild stage" where these kind of maps were resolved with strings.
One possible solution is to use global classes like and don't export any classes:
.layout {
& > :global(.regular) {...}
& > :global(.fullwidth) {...}
}
I didn't like this at first because you need to keep this knowledge in mind that you need to use THESE EXACT classnames.
For checking duplicated styles you may run this in the console and see the results:
Array.from(document.getElementsByTagName('link')).filter(link => link.rel === "stylesheet").map(l => l.href)
Looks like I fixed this problem only partially... continue investigating
Ok, another thing I discovered. Using load
function in route
definition leads to style duplication.
For instance we have 2 pages and each of them has route definition with load
function. As we see from the screenshot on search
page the styles for home
page are already there! All common things will be loaded N times (where N is number of pages with load
function)
UPDATE: not load
causes this, but in general the use of route
Updated example https://stackblitz.com/edit/github-amsqsj?file=src%2Froutes%2F(page-layout)%2Fsearch.tsx
More on style duplication: using common context across different pages will cause style duplication.
Imagine we have common service which can render some component on every page. In this case it doesn't really matter where this service is located (under router root or outside router). This service provides some API via context. Its styles will be duplicated N times (where N is amount of usage of this service).
<CommonService>
<Router root={(props) => <Suspense>{props.children}</Suspense>}>
<FileRoutes />
</Router>
</CommonService>
https://stackblitz.com/edit/github-amsqsj?file=src%2Fapp.tsx
You see, how many style duplications are there.
Ok, finally, I made it! I reproduced the bug with wrong order. @katywings
Check the example. There's ReusablePart
component. This component is shared between pages. Each page rewrites its styles by adding custom CSS (make text green). In dev it works correctly:
ReusablePart
In prod the situation is the following:
lazy
mechanism for routing.https://stackblitz.com/edit/github-amsqsj?file=src%2Fcomponents%2FReusablePart.tsx
@ryansolid @nksaraf in this issue I described bunch of critical bugs. They are important because they affect production. Please, pay attention to this. I'd like to help, but I need help to understand what's going on inside vinxi. For dev mode I managed to figure out, but for production build I just don't understand how it works.
Just as a quick info: I am currently investigating this. @elite174 thank you for all the information and reproduction, it's really is simplifying my work here :).
I gave up on solid start today and migrated to astro 😁
I am not the type of human who will try to convince you to come back, but its sad to see you go 🙈. Well.. and I still wanna solve your issues though 🤩 @elite174
IS: DEV and PROD have different CSS orders.
Currently dev seems to load CSS in the order of imports, while prod loads CSS by depth. @elite174 for your case in particular this means, if you move your Homepage styles import higher in the file, the text color will be consistently white, in DEV and in PROD, as you can see here: 2024-06-25_13-21
Recommendation (SHOULD): DEV and PROD should have the same CSS order logic.
IS: DEV loads by the order of imports, PROD loads by depth.
In my opinion the way that PROD currently orders the CSS is already the proper way. Comment making component styles more important than page styles
- this is right, component styles should absolutely be more important than page styles. Imagine if page styles were more important than component styles: are css resets also more important than page styles? Here are some ways how we can prioritize (sorted by priority DESC) and I think there is only one proper way 😅 (1.).
Recommendation (SHOULD): DEV should also load CSS by depth. @nksaraf We could change DEV to first append directly imported CSS, before further crawling nested javascript modules. This would align the DEV logic with the one from PROD.
The no-change alternative is to inform consumers in the docs, that they should import CSS before other components. This way they can manually make sure that CSS in DEV has the same order, as in PROD.
Now for @elite174, my recommendation would leave you with the white text from the module, because the module has the higher priority than the page. If you want that a page style overrides the module style, you can do this with common CSS priority tactics, for example you could do this: Bildschirmfoto vom 2024-06-25 14-07-29. Now the text is green, in DEV and PROD, because .a .b
has a higher priority than .c
, as you probably know 😅😉.
IS: Importing the same css file in multiple nested route files (a.k.a layouts) adds the file multiple times to the rendered html.
Each nested route file is called by lazyRoute
. The lazyRoute
calls do not know about each other. If a file is included in multiple nested route files, each lazyRoute
will have it in its assets. SolidStart currently does not deduplicate assets that appear in nested routes, it just appends everything https://github.com/solidjs/solid-start/blob/74ceb8076d12916526e305581ddb8416779be504/packages/start/src/router/lazyRoute.ts#L55.
lazyRoute
calls from a single request example:
lazyRoute -> src/routes/wrap.tsx?pick=default&pick=$css
lazyRoute -> src/routes/wrap/index.tsx?pick=default&pick=$css
Recommendation (SHOULD): SolidStart needs a way to deduplicate assets during SSR. My personal workaround for asset deduplication currently looks like this: https://codeberg.org/nitropage/nitropage/src/commit/dc8858a89e7ad484c252b06c2894574c622d7264/packages/nitropage/src/components/common/head.tsx#L20. @ryansolid Not sure if we could do something similar here, but I guess its best if we split this topic up into a separate issue in solid-start and discuss possible solutions there.
@katywings
CSS styles which used for overriding should be in the end (down) always. It should be WYSIWYG. At the moment in DEV the style order is correct, it should be the same in PROD. It works exactly like that in every framework: in solid (SPA), in Astro, in Next whatever. The only way to control the order of CSS is import order, that's why it's important.
So I don't agree with your opinion on CSS order. Please, check out other frameworks.
Imagine I want to override styles of component A in component B.
component B:
import {A} from '~/components/A';
import styles from './B.module.scss'
const B = () => (
<A class={styles.B} />
);
So firstly: import A and THEN apply styles of B (from TOP to BOTTOM). It doesn't really matter what B is - a component or a page or a layout.
@elite174 Well yeah, if every other framework is doing it the different way around, then we probably should do it like the others :), the important thing in the end is that DEV and PROD are doing it the same way, whatever way that is. I am not the one making the decisions here, I just give out recommendations 😅...
Yeah, I agree that there should not be any differences between dev and prod, because... what's the point of dev mode, if in the end you end up with something different?
The problem with the other way is the how: the manifest from Vite separates out css and js imports and therefore doesn't include information about the original import order. We use the Vite manifest in PROD to render the assets. Take a look at the structure of the vite manifest (https://vitejs.dev/guide/backend-integration). With this in mind, I am starting to wonder how every other SSR framework does this differently? Are literally none of them based on vites manifest?
Aligning PROD to DEV seems to be very hard with the manifest in mind, we would have to attach more information to the manifest somehow... Aligning DEV to PROD (as I suggested) would be comparatively easy.
It might be... Remix and the work Tanner has been doing treats all ambient CSS as global to my knowledge and push people to using special router metadata exports so they control it. So it is possible they let Vite just do its thing in these cases and Vite generates the right order somehow? Because as you said we don't have the information to know what JS is imported before what CSS from the manifest. And that's a Vite thing.
I just tried it out: in Remix (PROD), just like in Vinxi, the import order does not matter, but Remix seems to order exactly the opposite way:
In Remix (PROD) I get a link order like this (even though I imported in a different order)
<link href="root.css" />
<link href="component.css" />
<link href="route.css" />
Last but not least: the result in Remix is actually also inconsistent between DEV and PROD 😂.
Now about Astro: They definitely don't use the vanilla vite manifest and they have a whole extra sorting algorithm for this stuff:
Personally I see two ways to improve this in Vinxi:
Recommendation (SHOULD): DEV should also load CSS by depth. @nksaraf We could change DEV to first append directly imported CSS, before further crawling nested javascript modules. This would align the DEV logic with the one from PROD.
The no-change alternative is to inform consumers in the docs, that they should import CSS before other components. This way they can manually make sure that CSS in DEV has the same order, as in PROD.
I totally agree. Lets go by depth. Asserts order in both directions, deep and wide.
@nksaraf you are suggesting for the time being we replicate Remix behavior for both dev/prod? Ie.. JS first then local CSS. Ie.. CSS gets processed on the way up?
EDIT: Confirmed yes.
Probably best if someone of you implements this (@nksaraf / @ryansolid) because I feel like we are talking about different solutions 😅.
Yours: change PROD to align with Remix & change DEV to align with PROD.
Mine: change DEV to align with current PROD.
Personally I think traversing first (as remix does) is not what the user expects in most cases..
Usually you use page styles as a base layer and override them in the component styles. If you do depth first, all component styles have a lower priority then the page styles...
Ok. That is a point of debate.
Neither Nikhil nor I are that attached to the order here we just need to pick one. I suggested Remix to clarify since what he didn't make the decision clear in his response. But we both don't particularly feel there is a compelling reason to pick one over the other. But I want to get this unblocked because the likelihood that he addresses this in short order is pretty low. I will be looking at the deduping in SolidStart and probably won't touch this issue either.
The one justification of Remix order I can think of is that most people import js above CSS so it looks almost like it is import order to follow @elite174 expectation. I imagine this is why they never even realized Remix solution wasn't that smart. But neither of us are particularly attached to either direction. Changing DEV to match PROD would be equally great I think if there is a justification that we can give. Honestly it'd be great to get more feedback but I'm not sure we'd get enough to matter or quick enough to be worth delaying any potential fix someone might be so inclined to make.
Personally I think traversing first (as remix does) is not what the user expects in most cases.. Usually you use page styles as a base layer and override them in the component styles. If you do depth first, all component styles have a lower priority then the page styles...
We're not talking about page vs component styles. As I mentioned it's about component B overrides styles of component A, and it doesn't matter what component B is.
It interesting that discussion here is mostly about easy/hard but not about correct and intuitive. Yes, making things right is sometimes difficult, but that's the whole point. People read code from top to bottom, that's how our brain works. Importing styles which used to override at the bottom is a natural thing, that's why astro for instance implementing different algorithms to make it work, because that's how it should work.
Going on easy direction just because it's easy... Well, IMO it will add more "special concerns" about how I need to write the code for this particular framework. Extra mental model, extra rules to keep in mind. I think it would be one more step to solid-start become a framework that people will avoid just because it's not like the others and adds extra rules to your head.
@elite174 Easy vs Difficult always comes up because of limited resources. It sounds like you may be volunteering?
Thanks to the digging from @katywings it appears with exception of Astro who built their own system we are dealing with the default Vite behavior. Not everyone has bothered building their own CSS solution. I'm ok with a reasonable position that aligns with other solutions in the space in the short term since what we have right now is inconsistent which is arguably a lot more serious. We have other CSS fixes that need to get out ASAP and I'd rather have a reasonable stab at this now and release this sooner than waiting for something that may take months to come realistically if we don't have volunteers.
But wait a second, you were committed to make a meta-framework from scratch. It's definitely a tough task. Nobody really pushed for the 1.0 release, right? Overriding component styles in CSS is a basic thing, not a corner case. Style deduplication - also a basic thing.
There were no tests for that, and solid-start were released with 1.0. People start using it, and now all these problems occure and you're talking about limited resources. There is always be lack of resources.
Somewhere it was mentioned that 1.0 means nothing and it's just semver. Ok, then, I have a question: what version SS should have for correct work of basic scenarios? 1.17? 2.0? What is the line when we all can say that "Now SS covers all basic scenarios and ready for production. Of course there can be bugs but for corner cases."
This project has maintainers, not volunteers. If you release smth to public be prepared to take responsibility of maintaing it. I do help with some things of course, but I'm not a maintainer, not a member of solid-start team, I have a job and responsibility for another product.
Style deduplication is a bug and one that will be fixed soon.
The style import order is trickier given the underlying tools don't provide the necessary hooks to do so. Frameworks well past 1.0 (like Remix) haven't bothered fixing this so singling this out doesn't make much sense. Nor is calling something basic when it is clearly not. Additionally many people have used SolidStart and not hit this issue because of various other CSS approaches they take. I'm having a difficult time getting feedback on this issue in the discord because so few people have experienced it.
I've been very open about the lack of resources and it is baked into the strategy of using things like Nitro and Vite to accomplish our goals. To a certain degree if those tools don't give what is needed there are limits to how far realistically we can take things. If these tools didn't exist SolidStart wouldn't exist. That was always part of it. If these tools don't provide something SolidStart probably won't either. Vinxi has 1 maintainer and SolidStart has a couple. We are making efforts to change that, but it will take some time. Which is why in every communication I've emphasized the importance of contributors.
All maintainers are volunteers generally speaking. With exception to myself and where we can funnel OSS funds no one is being paid to maintain these projects. They all have other jobs and responsibilities. There will be always be lack of resources so as a user of OSS you take the risk always of the best fix taking longer than the one to move things forward.
To your 1.0 question. I'm not going to hide behind the Semver thing. We released 1.0 with the expectation of it being production ready. Obviously we've hit some bugs and we are addressing them. You may not like the fix or the prioritization, but our only commitment here is that we are open and transparent about what we can do and when we can do it to the best of our ability. That's all we've ever been able to do, and that hasn't changed.
You're trying to explain the situation, but it doesn't matter actually. Discussions about complex things are difficult to support, comparisons to others, OSS and volunteers "problem"...
They only thing which matters is does it work properly or not. If we talk about the web, there're 3 fundamental things: HTML, CSS, JS. How to make sure that your front-end framework works? Identify, what "works" mean considering these 3 parts, write tests, make things work for the tests. That's why I said that it's a "basic" scenario, because if you start using the framework without all fancy technologies like tailwind, pandacss or whatever, you'll identify these issues really quickly.
Bundling is fundamental, critical for meta-framework. It's an important part of it, that's why it should be reliable and well-covered with tests. Without that we need to question about the quality of SS and the future of SS at all. Does it really make sense to make a path for a new framework when quality and reliability are not a concern? I think it might be a dangerous path leading to failure.
I gonna pick up on https://github.com/nksaraf/vinxi/issues/318#issuecomment-2189924379 and leave these thoughts for the night:
All of that said I am still in favor of aligning DEV to PROD (deep-last), that seems to be what Vite Docs want.. But i'd love to hear Remixes reasonings for deep-first.
P.S. Whichever way we decide to do in the end, I like to help implementing the fix - I have my preferences, but I can adapt 🙏.
Screenshots from the Vite Backend Integration Docs:
Regaring https://github.com/nksaraf/vinxi/issues/318#issuecomment-2190315051
Another point to consider: Imagine in my org I have several projects (solid SPAs (just vite+solid) and solid-start SSRs). If I want to copy part of my code from one project to another I expect that it should work the same. In my opinion it will be weird to change order of CSS when copy-paste from SPA version and vice versa.
@katywings that sounds good. I've reached out to Mark. If I don't get a response I wouldn't sweat it either way. But I'm looking forward to this. Thanks for doing thorough research into this.
I just finished switching an existing project from Astro to solid-start and also noticed this issue. Most styles had to be rewritten to be more specific and not depend on import order. This is indeed very annoying and makes the UX for styles less friendly. I'm using panda-css and overriding component styles was pretty easy using Astro, but I need to always use a more specific selector in solid-start.
Having different behaviour between dev and production was also a surprise.
Maybe re-implementing the sorting algo that @katywings mentioned from Astro could be a good idea, to keep everything consistent between dev and production.
BTW thanks for the hard word, amazing project overall, very excited about the future of this whole part of the solid ecosystem !
The challenge with trying to replicate Astros approach is Vite doesn't generate the information needed to do it. So it's more than sorting. We'd need to crawl and scan the files ourselves and generate our own manifest.its doable but it's considerably more work to build and maintain a custom asset pipeline. Which is why others haven't done this I imagine.