systemjs / plugin-css

CSS loader plugin
MIT License
92 stars 60 forks source link

plugin-css requires a fresh builder instance each time #102

Open arackaf opened 8 years ago

arackaf commented 8 years ago

Hi Guy. I'm seeing what I think is a bug. Essentially, the plugin-css requires a fresh builder instance each time. If you create bundles with css dependencies on the same builder instance, some lines seem to get crossed, and css ends up in incorrect places.

Specifically, consider

for (let name of namesToOptimize) {
    let builder = new Builder('../' + baseDirectory);
    builder.config(configToUse);

with calls to

await builder
          .bundle(`${name}`, outputPath, configToUse)
          .then(result => {

below, inside the loop body. If I move

let builder = new Builder('../' + baseDirectory);
builder.config(configToUse);

above the for loop, the problems described above surface.

Is this expected behavior? Do you need me to repro this somewhere in GitHub?

guybedford commented 8 years ago

There may well be bugs with running multiple builds at the same time as the individual plugin instance is designed to work against a single build.

We may need some way to key the build instance itself in the internal plugin cache layer here.

arackaf commented 8 years ago

Simpler solution might be to just error out if the plugin is run more than once on the same builder instance.

guybedford commented 8 years ago

The issue shouldn't actually be the instance, but rather having multiple builds running at the same time?

Or even with waiting for the build to complete are you getting issues using the same builder instance?

arackaf commented 8 years ago

I just checked - every call to bundle is indeed preceded by await so I think there should never have been more than one running at the same time.

guybedford commented 8 years ago

I've done some further testing and can confirm this is entirely to do with parallel builds not being supported.

I think I have some ideas going forward, but it will require some changes to the builder API so don't want to rush this.

arackaf commented 8 years ago

This tool is pretty outstanding as is. Preventing parallel builds is easy with the simplest of helper methods - the best work to reward ratio may be to just error out on parallel build attempt. My $0.02 :)

guybedford commented 8 years ago

Thanks, unfortunately the detection problem is just as hard though!

guybedford commented 7 years ago

I've included a fix for this in https://github.com/systemjs/plugin-css/commit/6be8e6467acab811bb9fbceaf598a18ffa5f8b2d, although there may still be some race conditions remaining. Let me know if it helps though!

arackaf commented 7 years ago

Sure - thanks much!