Order of merged-stylesheets.css in <head>

SachaG commented 7 years ago

Migrated from: meteor/meteor#8651

SachaG commented 7 years ago

A pretty common use case if you're using a third-party UI framework such as Bootstrap is loading it from a CDN, for example using Helmet to inject link tags. .

The problem is that currently, Meteor's own stylesheet (merged-stylesheets.css) gets injected right at the top of the , meaning that Bootstrap's CSS will come afterwards and override your own user-defined styles.

So I would suggest modifying the boilerplate-generator package so that Meteor's own stylesheet always comes last. Are there any downsides to doing this?

aogaili commented 7 years ago

Any update on this?

rcurrier666 commented 7 years ago

@aliogalli, it appears the decision was made to not address it in the initial Boilerplate refactor (#8820 above) but instead to come back to it as a separate issue.

@abernix, please don't forget to come back around to the CSS load issue. My code is littered with class="myapp" additions to Bootstrap classes just so I can override the default Bootstrap formatting. Clutters up and confuses the code. Re the previous discussions, I'm happy with whatever solution is decided upon, though I would lean toward adding a replacement field, suggested by hwilson, to allow me to put the merged code wherever I want (not for any particular use case, just seems the most flexible). <link rel="stylesheet" href="METEOR_BUNDLED_CSS_URL" />

aogaili commented 7 years ago

I really like the proposed solution of using the <link rel="stylesheet" href="meteor_merged_css" /> placeholder.

The placement of the merged CSS has performance implications as well. Using SSR, bundle visualizer, Meteor 1.6 and dynamic imports I was able to drop the initial load time from 5 seconds to 1.4 seconds (thanks @benjamn!). The merged CSS is in the rendering critical path and it's delaying the app by the 0.5 seconds. It's also impacting the lighthouse performance results ( right now it's 52% but it could be 92% if I remove the non critical css and inline what's needed.)

So @abernix I think this change is crucial for developing a highly performing Meteor apps in addition to having more control over the style sheets priority.

aogaili commented 7 years ago

Here is a screenshot of where I currently stand. According to lighthouse, meteor merged css is delaying the initial paint by 590ms. I think if we close this issue, I can hit 100% on performance in lighthouse and this is a fairly complex app.

hwillson commented 7 years ago

Thanks for sharing your findings @aliogaili. Now that the boilerplate refactor has been completed, making the change outlined in https://github.com/meteor/meteor/issues/8651#issuecomment-300137401 is quite straightforward (actually, I made the necessary code changes a while back to test things out; they could be easily brought into the main repo). I'll bring this up at this week's issue triage meeting. More details shortly - thanks!

aogaili commented 7 years ago

Thanks @hwillson Just to confirm, with the proposed solution, would it be possible to place the link outside of the head block?

benjamn commented 7 years ago

Since the boilerplate generator technically allows for multiple CSS resources, I would tweak @hwillson's METEOR_BUNDLED_CSS_URL idea to use a placeholder HTML element instead:

<head>
  <title>meteor-issue-8651</title>
  <link
    rel="stylesheet"
    href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css"
    integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u"
    crossorigin="anonymous"
  />
  <meteor-bundled-css />
</head>

The <meteor-bundled-css/> element would usually be replaced by just one <link> tag, but could be replaced by more than one (or zero).

benjamn commented 7 years ago

And while we're at it, we might as well support <meteor-bundled-js/> as well!

aogaili commented 7 years ago

Perhaps we can think and name those placeholders as component (Meteor Component). The two components are <MeteorBundledCSS /> and the <MeteorBundledJS />. Those "Meteor Components" can be placed anywhere in the main.html and they can also have additional props in the future to control their rendering behaviour.

Developers are already used to the concept of components and perhaps it's useful to name and present them that way.

zimme commented 7 years ago

Developers are already used to the concept of components and perhaps it's useful to name and present them that way.

But as they aren't really components I personally don't think they should be "seen" as components which might confuse inexperienced developers.

aogaili commented 7 years ago

Fair point, I just thought I'd put it out there.

aogaili commented 7 years ago

But the concept of components has already been stretched out in the react ecosystem, think of react helmet, it's behaviour is not that different then the proposed Meteor tags. And inexperienced developers will probably not use those tags anyway, so perhaps it's debatable objection.

zimme commented 7 years ago

@aliogaili, what about angular developers using meteor or vue or some other view layer? They might be used to components looking different.

I think that this should be it's own thing which isn't something to relate to "components" because it's a part of Meteor's build system. But that's just my view of this.

aogaili commented 7 years ago

Got it @zimme I guess it's because I'm coming from react naming convention. So are you also objecting to what @benjamn proposed? because what he proposed in syntactically similar to a component (with vue naming convention) or are you just objecting on using react naming convention (which I agree that it might be confusing to some) ?

zimme commented 7 years ago

What he suggested is ok with me because that's closer to html element naming convention.

aogaili commented 7 years ago

I agree with you, I think it's better to keep closer to the html naming convention.

But I guess it's worth emphasizing, from conceptual perspective, that what's being proposed here is actually a custom html tags (a.k.a components) transpiled by Meteor build system and could potentially have additional props in future.

antoninadert commented 7 years ago

what @benjamn proposed looks great to me. I can easily imagine using it in my apps. The js version too. Can't wait for it

antoninadert commented 7 years ago

I just have a new comment: I love Meteor, but now that I think of it twice, I wish I was able to take full control of packages on the client-side.

The problem is that Meteor client build provides an abstraction layer that was supposed to make things simpler (goold old Blaze-only times) but now is actually more complicated (because if you open front-end to multiple stacks, then you need more control over them)

I totally believe we should be able to plug any front-end stack, and it should work with server-side rendering provided by the server-render package.

To maintain backward compat, the current build system should be isolated in a package, and removing this package should let me play with the frontend stack at will (think of it like insecure package)

I am totally PRO having some solution like meteor-client-bundler integrated in Meteor core and supported by MDG. At this stage I could just remove meteor's client-side packages carefully and it would be obvious what features will be missing by removing specific packages, by looking at the docs for example.

I would then be able to control my client-side script and css order entirely, even maybe to switch my builder to webpack for the front, and still be able to connect to any meteor backend via HTTP or DDP and get this reactivity, if I opt-in for some packages that enable that to me.

Well... Sorry if I am a dreamer

benjamn commented 7 years ago

@antoninadert I'm not sure where to begin convincing you of the value of Meteor's build system versus something like Webpack. What you're proposing no longer sounds like much of a framework at all, and I'm afraid I can't imagine that dream having more value than the current system for most Meteor developers.

Let's keep this issue focused on the JS/CSS tag order problem?

antoninadert commented 7 years ago

Sure !

aogaili commented 7 years ago

Alright so I think we're settling on Benjamn's suggestion of <meteor-bundled-css/>and <meteor-bundled-js/> I'm really looking forward to this since it'll help us to speed the initial load time.

antoninadert commented 6 years ago

Any news on this ? It would be so good to have it on 1.6.1

Meanwhile, is there any workaround to delay stylesheet loading ?

rcurrier666 commented 6 years ago

Bump... This didn't seem to make it into 1.6.1 nor is it on the lists for 1.6.2. Since there have already been more than one experimental implementations, is there a reason this feature hasn't been included? Having to add another class to all of my Bootstrap elements just so I can actually override the default Bootstrap CSS is a huge hassle and clutters up the code.

hwillson commented 6 years ago

is there a reason this feature hasn't been included?

@rcurrier666 The core team is caught up working on other great features, but this FR is pull-requests-encouraged. If you're interested in helping out (using https://github.com/meteor/meteor-feature-requests/issues/24#issuecomment-327527616 as a guide), we'll definitely review a PR. Thanks!

SachaG commented 6 years ago

This is how I fix the issue in my own version of the package, if that helps: https://github.com/VulcanJS/Vulcan/commit/08c0e9056f149f80bb42f2978bc429578285a4c5

rcurrier666 commented 6 years ago

@hwillson, et al; since I have some spare time and the inclination to do so, I'm putting together a PR for this feature. But we need to resolve exactly what this feature looks like. There are 3 competing options:

Move the CSS bundle to the end of the head. PROS: Solves the general problem, trivial to implement. CONS: Are there any cases where this will break existing apps?
Use a "component" tag such as<cssbundleatbottom> as a flag to put the CSS bundle at the bottom. If not present, CSS bundle goes at the top. PROS: maintains backward compatibility, relatively easy to implement, can be placed anywhere in the head CONS: Less flexible than option 3
Use <link rel="stylesheet" href="meteor_merged_css" /> to indicate where in the head to place the CSS bundle. Could instead use a "component" tag such as <putcsshere> If not present, then place it at the top to be compatible. PROS: Completely flexible for all use cases, maintains backward compatibility. CONS: Somewhat complex to implement, I don't actually see a use case for this much flexibility as opposed to option 2, but someone might.

I have working (but ugly) versions of all 3 already coded. If you have a preference, please leave a comment. If there isn't a consensus by Feb 10th, I'm going to go with option 3 since it is the most flexible. I've cross-posted this [slaps his own hand] in the forums to try to maximize the number of responses.

derwaldgeist commented 6 years ago

I personally think that if loading order of CSS files matters, your CSS code is messed up. It is the purpose of the "cascading" part in CSS that you can override your styles as needed, your styles just have to be specific enough.

The simplest solution to achieve this is to add a style class to your <html> tag like this:

<html class="my-awesome-app">

You can now override any Bootstrap style easily by prepending it with .my-awesome-app, e.g.:

.my-awesome-app .btn-primary {
   background-color: magenta;
}

A second approach is to use the SASS version of Bootstrap which allows you to customize Bootstrap via variables.

In my own apps, I am using a combination of the two approaches and am very happy with this solution.

Another benefit of the "class-on-html-tag" approach is that it can be applied to user use-cases, too. For instance, I am defining additional classes depending on the environment the app runs in. If the app runs on iOS, I add the ios class, no Android android is added etc. This is very helpful if you want to design your Cordova apps according to the OS-specific style-guides.

SachaG commented 6 years ago

I personally think that if loading order of CSS files matters, your CSS code is messed up.

In that case you won't mind if we change it to be the other way around ;)

But seriously, I think relying on source order in CSS is pretty common, it helps avoid selector inflation. Making selectors more specific than needed is generally a bad idea anyway, and sometimes it's just not practical. For example, what if you're using a third-party bootstrap theme stylesheet and you need to ensure it always load after the main bootstrap CSS? You can't really just prepend every single class with .my-awesome-app…

rcurrier666 commented 6 years ago

To imply that CSS load order doesn't or shouldn't matter is simply incorrect. It's part of the CSS specification. See Section 6 of this W3C document entitled Order of Appearance. And, yes, you can overcome the load order by adding additional selector specificity, but that clutters up the CSS with extra selectors that could be avoided if the Meteor load order is changed. What I'm interested in most, is there a use case where changing the order of the CSS bundle from first to last will break existing code. If the developer has already worked around the issue by adding selectors, moving the bundle to the end shouldn't break anything. But is there a case where the existing order matters and would break code if the order is reversed? I'm looking for a reason to not simply implement case 1 (which involves moving 4 lines of code from the top to the bottom of the template). Otherwise, options 2 and 3 would suit your viewpoint since if you don't modify your code everything remains the same.

SachaG commented 6 years ago

I can't really think of a scenario where option 1 would break people's code, but with CSS who knows… It's worth pointing out that it's really easy to copy the package's code locally and change it manually if you need to.

derwaldgeist commented 6 years ago

There might be cases where the simple change in load order breaks existing styles, since load order matters in CSS. Imagine a developer who had set styles for an html element like p and a third party library had overwritten this. This might be inadvertently, but changing the load order would change the styling immediately.

Having said that, I think that putting the user-defined styles last is what developers would expect in the first place, since some just rely on the order of appearance and believe their own styles should always „win“.

The only problem is that Meteor decided to do it the other way around, and I am a big fan of backwards compatibility unless it isn’t avoidable. I had so many cases in the past when a simple meteor update broke my apps that I became somewhat sensitive for this.

derwaldgeist commented 6 years ago

@SachaG in principle, you’re right. That’s one of the reasons why I love Sass, since you can easily nest your styles then. However, this doesn’t always help. For instance, I had a third party library that made its own styles more specific in one of its minor releases which even broke the nested styles approach, and this would also break the styles if you had relied on loading order. In essence, there’s no good way to solve these kind of problems, since it is inherent in the way CSS works.

hwillson commented 6 years ago

Thanks for looking into this further @rcurrier666. Ideally we'd like to see @benjamn's approach mentioned in https://github.com/meteor/meteor-feature-requests/issues/24#issuecomment-327527616 and https://github.com/meteor/meteor-feature-requests/issues/24#issuecomment-327528168 implemented. With this approach a developer can control the location of Meteor's CSS bundle by putting the <meteor-bundled-css /> element wherever they want the bundle (which could be made up of several CSS resources) to appear. So top of <head />, bottom of <head />, top of <body />, etc. If <meteor-bundled-css /> isn't used, then Meteor's current approach would be used (for backwards compatibility). In addition, we'd like to do the same thing for bundled JS resources as well, using <meteor-bundled-js/> (but this could be handled in a separate PR if/as needed). Thanks!

rcurrier666 commented 6 years ago

@hwillson, glad we agree on the approach! While simply moving the bundle to the end is a trivial change, it is bound to break someone's app. A couple of questions for you;

Is there a use case for putting the bundle in the <body />? It would require either moving the body code from the closeTemplate back into the headTemplate (which is how the Cordova version is structured), or passing some sort of flag into headTemplate to let it know not to emit the <link /> tag for the bundle (presumably by looking into the body for the <meteor-bundled-css /> tag (which would mean the generator would need to know about the 'magic' tag.
Do we want the same feature in Cordova builds?
I'll do the bundled JS changes as a separate PR. Since the implementation is similar, let's get the CSS PR done and approved so I can just reproduce the changes for JS (and so one or the other can be backed out if need be.)

Look for a PR later in the week. And please bear with me, it's only my second PR, so I may need some guidance if I get it wrong.

skirunman commented 6 years ago

I'll answer

Do we want the same feature in Cordova builds?

Yes, please!

hwillson commented 6 years ago

I agree with @skirunman on #2. Regarding #1, I think if we can give the flexibility to include the bundled css anywhere someone might want to (in <head />, <body />, etc.), that would be ideal. Sounds good about #3 as well. Thanks @rcurrier666!

rcurrier666 commented 6 years ago

There is a major problem with inserting the CSS bundle in the body and with supporting moving around the JS bundle. If you've ever looked at the page source in the browser, you'll see there is NO HTML in the body, just <script> tags. The HTML is generated by the app/app.js script. It should be possible to move the 60+ <script> tags into the head and leave the app/app.js script in the body using a <meteor-bundled-js> tag in the head, if that is the desired behavior. Moving the CSS bundle into the body with <meteor-bundled-css /> doesn't seem possible since the body template can't see any HTML. We could, I suppose, use a different tag like <meteor-bundled-css-in-body> that is placed in the head and will put the CSS bundle at the top of the body before any <script> tags.

Unless someone likes the special tag to move the CSS bundle into the body, or has another idea on how to handle it, I'm going to limit the bundled CSS to the head.

And since I won't be looking at moving the bundled JS until after the CSS PR is complete, I'm going to open a new issue to discuss moving the bundled JS. That way the two features can be kept separate.

rcurrier666 commented 6 years ago

Could someone provide me with details of how to run the tests in /meteor/packages/boilerplate-generator-tests. I want to make sure I haven't broken anything plus I need to add new tests (if I can figure out how to test my changes). TIA.

abernix commented 6 years ago

@rcurrier666 You should be able to run those tests with the instructions in the Running specific tests section of DEVELOPMENT.md, found in the root of the repository.

So in your case you'd run:

./meteor test-packages boilerplate-generator-tests

rcurrier666 commented 6 years ago

@abernix, thanks. I tried that last night and it didn't work. I discovered that my Ubuntu image was missing some packages. After adding them, the tests work fine. Thanks for bearing with me, I've spent the last 7 years in a Windows + Perforce environment and so I'm dealing with a bit of a culture shock.

rcurrier666 commented 6 years ago

This feature has been submitted as PR #9657

rcurrier666 commented 6 years ago

Just a note that the changes for both browser and mobile are now in the PR in case anyone wants to take a look at them or try them out. I'm waiting to hear back on where to document this feature and then hopefully one of the powers-that-be will merge the PR (maybe we can get it in 1.6.2?)

rcurrier666 commented 6 years ago

All, I've opened a new issue to discuss implementing <meteor-bundled-js> to allow the JS scripts to be moved into the head. If you have any interest in this feature please head over here and chime in. I'm not going to implement this, and the powers-that-be won't merge the PR, unless there is actually some interest in the feature.

antoninadert commented 6 years ago

I believe there should be a way to mark JS or CSS to be merged in specific package and then there would be 2 package position options: either in HTML < head> either in HTML end of < body>

This is a standard way to position HTML < Script> and < Style> tags

SachaG commented 6 years ago

The new meteor-bundled-css thing isn't working for me. In template-web-browser.js, head is blank and all my head content is in dynamicHead, which doesn't get parsed for <meteor-bundled-css/>. I'm not super clear what the differences are between head and dynamicHead, I'm guessing it has something to do with the SSR process?

rcurrier666 commented 6 years ago

Since I did the original work, I'll take a look at it unless one of the MDG folks wants to do so.

= Ron

On 6/9/2018 6:27 PM, Sacha Greif wrote:

The new |meteor-bundled-css| thing isn't working for me. In |template-web-browser.js|, |head| is blank and all my head content is in |dynamicHead|, which doesn't get parsed for ||. I'm not super clear what the differences are between |head| and |dynamicHead|, I'm guessing it has something to do with the SSR process?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/meteor/meteor-feature-requests/issues/24#issuecomment-396012287, or mute the thread https://github.com/notifications/unsubscribe-auth/ADCTXGi6V1MmeKVXyKAzBulUquptwjRcks5t7HYPgaJpZM4NyHbO.

rcurrier666 commented 6 years ago

I've reproduced the problem (using juliancwirko/scotty boilerplate) and have a browser fix coded and manually tested. I still need to fix the cordova template and write some unit-tests to cover the new cases.

@SachaG, would you please open an issue for this so it can be tracked? I'll then do a PR for the fixes.

SachaG commented 6 years ago

Done: https://github.com/meteor/meteor/issues/9982

skirunman commented 5 years ago

Is this not closed as this implemented in https://github.com/meteor/meteor/pull/9657 and released in Meteor 1.7. Understand there is still out outstanding bug.

meteor / meteor-feature-requests

Order of merged-stylesheets.css in <head> #24