Question on CSS order of inclusion

[leslc]

I currently have a page where a base CSS file is the dependency of two separate modules. But the CSS bundle created is not what I expected. Is this a bug, or working as expected?

PAGE:

requires("header")
requires("footer")
// also has page CSS specified in "style of its package.json

header module:

requires("base-css");
// also has header CSS specified in "style" of its package.json

footer module:

requires("base-css");
// also has footer CSS specified in "style" of its package.json

When the CSS is generated for the page, I expected this order in the bundled CSS file:

1. base-css
2. header
3. footer
4. page

However, I get this unexpected order:

1. header
2. base-css
3. footer
4. page

If I switch the order of the "require" lines in PAGE, I still get the same unexpected order.

requires("header")
requires("footer")

However, if I only include one of the files in PAGE,

requires("header")

the order is correctly:

1. base-css
2. header
3. page

Is this a bug, or working as expected? If working as expected, can you tell me a little about the algorithm that determines the order of the CSS bundle?

Thanks!

[leslc]

@dgbeck, I think this is caused by "calcSortedDependencies".

There's a line https://github.com/rotundasoftware/parcelify/blob/master/lib/parcel.js#L35 that says it's there to avoid cycles. However, this line also has a side effect of skipping dependencies. If I comment out that line, the topograph is generated correctly.

What was this line trying to avoid and can we safely remove it to get the full dependencies?

[dgbeck]

Hi @leslc!

Interesting. We do need the logic in there to avoid cycles. But if this block is changed to

    return thisPackage.dependencies.reduce( function( memo, thisDependentPackage ) {
        var edges = memo.concat( [ [ thisPackage, thisDependentPackage ] ] );
        if( ! _.contains( visited, thisDependentPackage ) ) {  // avoid cycles
            edges = edges.concat( getEdgesForPackageDependencyGraph( thisDependentPackage ) );
        }

        return edges;
    }, [] );

Do you get the expected behavior and do all tests pass?

[leslc]

@dgbeck, yes, this works! (Ironically, I JUST tried this and was about to post back here :smile: )

Thanks, can you release as a patch?

[dgbeck]

Hm, unfortunately it looks like this does not eliminate the possibility of cycles. Does this modified version solve the problem?

        return thisPackage.dependencies.reduce( function( memo, thisDependentPackage ) {
            if( _.contains( visited, thisPackage ) && _.contains( visited, thisDependentPackage ) ) return memo; // avoid cycles

            var edges = memo.concat( [ [ thisPackage, thisDependentPackage ] ] );
            edges = edges.concat( getEdgesForPackageDependencyGraph( thisDependentPackage ) );

            return edges;
        }, [] );

[leslc]

Hi @dgbeck, thanks for taking a closer look. Unfortunately, this leads an incorrect order of inclusion as well. (It's a bit worse because the "base-css" dependency is not included before any modules that require it)

1. header
2. footer
3. base-css
4. page

I'm not sure of the exact workings but intuitively it seems that the dependency needs to be added first. Then the recurse into "getEdgesForPackageDependencyGraph" is prevented if it has been traversed already. (?)

I've been looking at the "edges" variable to see all the dependencies are there. What type of cycles are you seeing?

[dgbeck]

Hi @leslc ,

I can't seem to reproduce this issue. Check out test 7 in the test-32 branch,

https://github.com/rotundasoftware/parcelify/tree/test-32/test/page7

The output is

p {
    color: red;
}p.header {
    color: blue;
}p.footer {
    color: green;
}div.beep {
    color : green;
}

(The test-32 branch is using the updated logic that checks to make sure both thisPackage and thisDependent package have been visited, but it looks like that test is passing even with the old logic.) Can you try to isolate the issue further?

David

[leslc]

Thanks for setting this test case up. In footer/index.js, can you change to require "base-css"?

In re reading my initial report I think I missed an important part of PAGE to reproduce this. Apologies on that. It should be:

PAGE:

require('base-css')
require('header')
require('footer')

Hope that will help isolate this. Thanks again!

[dgbeck]

gotcha, thanks!

This should be fixed in v1.1.1, recently pushed and published to npm. Was a little more complicated then our initial hope but I think this is the correct general solution.

Thanks for pointing out this issue!

[leslc]

Thanks @dgbeck! Your solution definitely looks more robust. I'll try this out next week. Thanks and have a good weekend!

[leslc]

Got a chance to try this out, and the bundled CSS looks good. Thanks for fixing and releasing the patch so quickly!

[dgbeck]

Nice! Thanks for reporting & detailed info to reproduce!