rotundasoftware / parcelify

Add css to your npm modules consumed with browserify.
MIT License
251 stars 20 forks source link

Non-deterministic order of CSS inclusion, another case #45

Closed jreichenberg closed 8 years ago

jreichenberg commented 8 years ago

@dgbeck The original use case in https://github.com/rotundasoftware/parcelify/issues/32 seems resolved, but there is still some non-determinism in the inclusion order of CSS (and likely JS). Can you confirm whether you'd expect for a module with the js

require( 'my-module' ); // has CSS .my-module {color: red}
require( 'my-other-module' ); // has CSS .my-other-module {color: blue}

the inclusion order of bundle CSS should be

.my-module {color: red}
.my-other-module {color: blue}

Right?

We are seeing that no matter the order of require statements in the top level module the resulting CSS is always my-other-module's css first. (seems to be reverse alpha order)

Would appreciate a comment on whether our expectations are correct before I test case and PR this.

Here's the outline of a possible fix: Remove the sort() from https://github.com/rotundasoftware/parcel-map/blob/v3.0.0/index.js#L114 and change https://github.com/rotundasoftware/parcelify/blob/v2.1.0/lib/parcel.js#L56 to add (yet another) reverse() to the edges before toposort:

var sortedPackages = toposort( edges.reverse() ).reverse();

Works for the case above, but questionable whether this is a general fix. Thoughts? Thanks!

jreichenberg commented 8 years ago

Turns out my suggested fix in parcel-map above results in non-deterministic behavior. The current code is deterministic, but always results in reverse-alphabetical order of dependencies vs. the order in which they were require()-ed.

I spent a while on it, but couldn't crack it. I'd like to confirm the expected behavior before moving forward.

dgbeck commented 8 years ago

Hi @jreichenberg ,

I had not run into or even considered this case previously, so the expectation was not well defined. The behavior you are describing makes sense and I think with some serious work, parcelify could guarentee that the css is bundled with consideration of the js order-of-appearancem, assuming browserify supplies the js dependencies in that order. If I could wave a wand and make it so, I would.

However this looks like a very complex problem. Guaranteeing that order-of-appearance is preserved through the toposort is not at all straight forward. Working through this issue seems like a good way to loose your mind.

You are welcome to submit a PR if you can pull it off, but I image it would be a much easier path way to restructure the dependency graph so you don't run into this issue, or use more css specificity in my-other-module, so the order doesn't matter, if that's possible.

jreichenberg commented 8 years ago

@dgbeck Thanks for your thoughts. Yep, looks like some major surgery would need to happen in parcelify. We can work around by relying more on Sass/Less @ imports rather than order of require() for our cases.

dgbeck commented 8 years ago

great, thanks for the followup, great to know you can work around. closing for now.