-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Loader]: Loading CSS modules, fixes #1680 #1705
Conversation
CLA is valid! |
@@ -2182,7 +2189,12 @@ Y.log('Undefined module: ' + mname + ', matched a pattern: ' + | |||
} | |||
} | |||
|
|||
this.sorted.push(name); | |||
var isExternalCSS = module && module.ext && module.type === "css"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: a number of var
.
Thanks, @okuryu. |
|
||
// External CSS modules must be inserted last, | ||
// after all YUI related CSS. | ||
if (this._externalCSS.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worry about this. By adding _externalCSS
to the loader instance (similar to ), we might get into some race conditions when multiple calls to load new modules might get mixed. With CSS this is not a big problem because it is sync I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably if we rename the variable, that will be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your concern. As for multiple calls to load new modules-- I cannot imagine a scenario in which an error would occur.
_externalCSS
seems reasonable to me. Can you elaborate?
The more I think about this, the more I believe we should keep it as it is, and if they want to have explicit definition, they should call it out in the dependencies. |
CSS modules the user creates should load after any YUI related CSS. This is in case the user wishes to override the default styles of a YUI module, for example.
This feature was missed in the recent
Loader
changes due to no test being written for it.(fixes #1680)