-
-
Notifications
You must be signed in to change notification settings - Fork 470
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
fix(addStyles): revert merged loops #236
Conversation
@@ -102,14 +102,13 @@ function addStylesToDom (styles, options) { | |||
if(domStyle) { | |||
domStyle.refs++; | |||
|
|||
for(var j = 0; j < domStyle.parts.length && item.parts.length; j++) { |
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.
Why we do this? 😄
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.
Because I followed your guidance #224 (comment) 😛 🚀
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.
@michael-ciniawsky I'm ashamed, a lot of issues 😞 Btw, we need add tests for this, someone in the future may want to do the same and repeat our my mistake
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.
totally wayne 😛 , I'm still wondering while this actually makes a difference ¯_(ツ)_/¯ test passed on both ways, but seem to not cover this :D
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.
@michael-ciniawsky have we minimal test repo? maybe we can determine when this happens and add a tests
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.
Nope I haven't, only used it locally for a few days and it seemed to work fine + style-loader
tests. But I'm not using webpack-hot-middleware
or the like
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.
@michael-ciniawsky let's wait when we have test repo, then add tests and merge and publish. We have big problems with the tests (in many webpack-contrib
repos), if we continue to ignore this, it will be very bad
Fixes #234, #235