-
Notifications
You must be signed in to change notification settings - Fork 14
chore: migrate off of css-selector-tokenizer #9
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 99.58% 99.62% +0.04%
==========================================
Files 2 2
Lines 243 270 +27
Branches 71 82 +11
==========================================
+ Hits 242 269 +27
Misses 1 1
Continue to review full report at Codecov.
|
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.
Great job! Somebody else or we can merge?
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.
Just small fix
"semi": true, | ||
"singleQuote": true, | ||
"trailingComma": "es5" | ||
}, | ||
"dependencies": { | ||
"css-selector-tokenizer": "^0.7.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.
Remove this from deps and update lock file 👍
/cc @jquense ready for merge? Please fix small note |
I will continue development, merge this as is |
thanks! sorry didn't have a moment to finish it up in the last two days! |
/cc @jquense just clarify: all finished in this PR? |
yup, just the note you said |
@jquense i will start testing new PR with css-loader |
This is pretty hard :/ And i don't think there is way to be super confident that there isn't some subtle change in behavior due to api differences, unclear behavior, limited tests.
I think actually the right thing to do here is REMOVE support for the non-nested selector. the open ended selector is weird and inconsistent, even in the logic of this plugin there is unclear transformations. and I even added a test to ensure i wasn't changing something but the old behavior that is definitely wrong.