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

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

Closed
SachaG opened this Issue Jun 7, 2017 · 50 comments

Comments

Projects
None yet
10 participants
@SachaG

SachaG commented Jun 7, 2017

Migrated from: meteor/meteor#8651

@SachaG

This comment has been minimized.

SachaG commented Jun 7, 2017

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?

@aliogaili

This comment has been minimized.

aliogaili commented Aug 26, 2017

Any update on this?

@rcurrier666

This comment has been minimized.

rcurrier666 commented Aug 27, 2017

@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" />

@aliogaili

This comment has been minimized.

aliogaili commented Aug 29, 2017

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.

@aliogaili

This comment has been minimized.

aliogaili commented Aug 30, 2017

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.

image

image

@hwillson hwillson added the review label Sep 5, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Sep 5, 2017

Thanks for sharing your findings @aliogaili. Now that the boilerplate refactor has been completed, making the change outlined in meteor/meteor#8651 (comment) 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!

@aliogaili

This comment has been minimized.

aliogaili commented Sep 5, 2017

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

@benjamn

This comment has been minimized.

Member

benjamn commented Sep 6, 2017

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

This comment has been minimized.

Member

benjamn commented Sep 6, 2017

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

@aliogaili

This comment has been minimized.

aliogaili commented Sep 6, 2017

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

This comment has been minimized.

zimme commented Sep 6, 2017

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.

@aliogaili

This comment has been minimized.

aliogaili commented Sep 6, 2017

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

@aliogaili

This comment has been minimized.

aliogaili commented Sep 6, 2017

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

This comment has been minimized.

zimme commented Sep 6, 2017

@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.

@aliogaili

This comment has been minimized.

aliogaili commented Sep 6, 2017

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

This comment has been minimized.

zimme commented Sep 6, 2017

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

@aliogaili

This comment has been minimized.

aliogaili commented Sep 6, 2017

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

This comment has been minimized.

antoninadert commented Sep 7, 2017

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

This comment has been minimized.

antoninadert commented Sep 8, 2017

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

This comment has been minimized.

Member

benjamn commented Sep 8, 2017

@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

This comment has been minimized.

antoninadert commented Sep 9, 2017

Sure !

@rcurrier666

This comment has been minimized.

rcurrier666 commented Feb 2, 2018

@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:

  1. 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?
  2. 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
  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

This comment has been minimized.

derwaldgeist commented Feb 3, 2018

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

This comment has been minimized.

SachaG commented Feb 3, 2018

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

This comment has been minimized.

rcurrier666 commented Feb 3, 2018

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

This comment has been minimized.

SachaG commented Feb 3, 2018

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

This comment has been minimized.

derwaldgeist commented Feb 3, 2018

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

This comment has been minimized.

derwaldgeist commented Feb 3, 2018

@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

This comment has been minimized.

Member

hwillson commented Feb 4, 2018

Thanks for looking into this further @rcurrier666. Ideally we'd like to see @benjamn's approach mentioned in #24 (comment) and #24 (comment) 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

This comment has been minimized.

rcurrier666 commented Feb 4, 2018

@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;

  1. 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.

  2. Do we want the same feature in Cordova builds?

  3. 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

This comment has been minimized.

skirunman commented Feb 5, 2018

I'll answer

  1. Do we want the same feature in Cordova builds?

Yes, please!

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 5, 2018

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

This comment has been minimized.

rcurrier666 commented Feb 5, 2018

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

This comment has been minimized.

rcurrier666 commented Feb 7, 2018

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

This comment has been minimized.

Member

abernix commented Feb 7, 2018

@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

This comment has been minimized.

rcurrier666 commented Feb 8, 2018

@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

This comment has been minimized.

rcurrier666 commented Feb 10, 2018

This feature has been submitted as PR #9657

@rcurrier666

This comment has been minimized.

rcurrier666 commented Feb 13, 2018

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

This comment has been minimized.

rcurrier666 commented Feb 13, 2018

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.

benjamn added a commit to meteor/meteor that referenced this issue Feb 21, 2018

@antoninadert

This comment has been minimized.

antoninadert commented Mar 13, 2018

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

This comment has been minimized.

SachaG commented Jun 10, 2018

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

This comment has been minimized.

rcurrier666 commented Jun 10, 2018

@rcurrier666

This comment has been minimized.

rcurrier666 commented Jun 12, 2018

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

This comment has been minimized.

SachaG commented Jun 12, 2018

@skirunman

This comment has been minimized.

skirunman commented Oct 12, 2018

Is this not closed as this implemented in meteor/meteor#9657 and released in Meteor 1.7. Understand there is still out outstanding bug.

@benjamn benjamn closed this Oct 13, 2018

@benjamn benjamn added this to the Meteor 1.7 milestone Oct 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment