geelen / jspm-loader-css

50 stars 27 forks source link

bug: strange import order some times... #13

Closed douglasduteil closed 9 years ago

douglasduteil commented 9 years ago

Hi @geelen

I'm having a strange import order time to time when sharing the same stylesheet with two components.

I'm a bit confused not giving you directly a working buggy demo but please if you can take the time to refresh this demo several times (with or without cache) you will notice an unusual pink background on some buttons that results to a wrong style tags order in the head.

geelen commented 9 years ago

First up, 100 internet high-fives to you for sending a Plunkr. That is so freaking helpful.

Also, you have found a FANTASTIC bug. Fantastic in the awful sense. :sob:

Ok, so here's what's happening:

At the top of your main.js you have:

import styles from './main.css!'
import primaryButton from './primary-button/primary-button';
import dangerButton from './danger-button/danger-button';

The first line triggers JSPM to go and try to find a plugin for css format, which ends up finding jspm-loader-css-modules and grabbing it from the CDN. That takes long enough that the files primary-button.js and danger-button.js get loaded as well. They have their own CSS files to import, so they both get queued, or something, waiting for css to be sorted out.

When css gets resolved, all three of them fire at once:

image

but it's a race to see which comes back first. For example, this is what it looks like when danger-button.css wins the race:

image

You end up with this ordering in the DOM:

image

And this output:

image

Now, main.css always appears before the other two, since that's something I explicitly ensure. But when danger-button.css triggers the download of button.css, my algorithm explicitly says "put button.css directly above danger-button.css in the DOM". And then when something else uses button.css, I assume it's in the right place, and I change nothing.

I honestly can't believe you found a way to reproduce this. If you need a quick fix, add:

import "./boostrap/button.css!"

to the top of main.js.

I'm gonna work on a real fix now though :)

geelen commented 9 years ago

FIXED!

Figured out how to recreate the situation. By injecting random delay in the loader

    return fetch( load ).then( source => {
      let delay = Math.round( Math.random() * 5000 )
      console.log( `Delaying ${path} for ${delay}ms` )
      return new Promise( res => {
        setTimeout( () => res( source ), delay )
      } )
    } ).then( source => {

You can get the files to be processed in a non-deterministic order each time. Which reproduces the problem, so I could fix it:

restore-order

So you can see the button.css file being bumped up as we find out we loaded it out-of-order. Will clean this up and release a new version :)

geelen commented 9 years ago

Deployed v0.1.6. Not sure how long it'll take to hit the CDN, but your plunkr should magically fix itself when it does :)

douglasduteil commented 9 years ago

Hi @geelen I'm so happy I could help on this ! :)

I started a fork of your code to understand how things works. And i made an experimentation with debouce and toposort. What you think ? http://embed.plnkr.co/Zhu2MO

geelen commented 9 years ago

That's nice! So nice I'd probably merge it if you PR-ed it :)

There's just a couple of things. You don't want to re-inject all the CSS on every change, the browser then has to pick up and reparse the whole thing. I don't know what the impact of that would be over inserting/moving a new style tag, but I'd want to see some performance numbers before agreeing to change my approach.

The other thing is that <link> tags using ObjectURLs give a better development experience. See https://github.com/systemjs/plugin-css/issues/25#issuecomment-88713442

And finally, there needs to be support for running in NodeJS for bundling. But if you could solve those, I really like the way you use toposort, so I'd love to move across to it :)