search

dojo / util

Dojo 1 - build utilities. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
60 stars 105 forks source link

fix #17429 optimizeCss can produce invalid CSS for unresolved @imports #17

Closed treasonx closed 10 years ago

treasonx commented 10 years ago
dylans commented 10 years ago

This is covered under the SitePen CLA... @treasonx is there a ticket number in bugs.dojotoolkit.org for this?

@rcgill can you look at this soon?

treasonx commented 10 years ago

Sorry forgot to add a link to the ticket.

https://bugs.dojotoolkit.org/ticket/17429

brandonpayton commented 10 years ago

These changes look reasonable to me but definitely need to be reviewed by @rcgill.

One thing I am wondering: Should it be configurable whether unresolved imports are errors or warnings?

csnover commented 10 years ago

One thing I am wondering: Should it be configurable whether unresolved imports are errors or warnings?

No, why should it be? It does not prevent the system from working.

brandonpayton commented 10 years ago

Depending on the app, you might believe that all of an app's CSS is present at build time. If it is not and is unable to be built with the other CSS resources, I would consider that an error and would like the build process to exit with a non-zero exit code so any build automation I am using considers it a failure. Then the issue can be identified and fixed sooner rather than later.

csnover commented 10 years ago

Sure, but you could say the same thing about literally everything else that warns right now. Missing build plugins, unresolvable plugin resources, etc.

brandonpayton commented 10 years ago

Ok, I see we might want to wait to tighten all these things at once rather than being inconsistently configurable.

treasonx commented 10 years ago

The reason it was an error before was due to the fact that you could end up with invalid CSS if an import couldn't be resolved at build time. Now that we hoist the imports not being able to resolve an import is a warning condition.

I don't think we should add an option to make it an error condition. Adding too many options will make the build tool harder to configure and use. We already seem to have a lot of undocumented features, I wouldn't want to add to the complexity.

csnover commented 10 years ago

Landed in 27a481239d94044b93053a82bfd82518facb5e8d. Should also fix some issue with the demos css mentioned in the dojo-meeting yesterday.