Skip to content
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: prevent merging of :host(tag) and tag #876

Closed
wants to merge 5 commits into from

Conversation

anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Feb 13, 2020

fixes #730
closes #731

@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #876 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
+ Coverage   97.33%   97.33%   +<.01%     
==========================================
  Files         118      118              
  Lines        3452     3455       +3     
  Branches     1035     1036       +1     
==========================================
+ Hits         3360     3363       +3     
  Misses         84       84              
  Partials        8        8
Impacted Files Coverage Δ
packages/postcss-merge-rules/src/index.js 97.74% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 402b2b3...3fc763d. Read the comment docs.

@anikethsaha
Copy link
Member Author

increasing jest timeout to 30s to fix this and for future changes as well

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2020

Codecov Report

Merging #876 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #876   +/-   ##
=======================================
  Coverage   97.28%   97.28%           
=======================================
  Files         118      118           
  Lines        3458     3461    +3     
  Branches     1040     1041    +1     
=======================================
+ Hits         3364     3367    +3     
  Misses         86       86           
  Partials        8        8           
Impacted Files Coverage Δ
packages/postcss-merge-rules/src/index.js 97.74% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f326b7f...0da5aec. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is safe to merge, but not for all cases, it is unsafe for browsers which not supported :host https://caniuse.com/#search=host%20selector, so we should use browserslist here to ensure we can merge it or not

@anikethsaha
Copy link
Member Author

@evilebottnawi

I think this fix is not required as we are already checking the compatibility.
I could write a test for IE 11 env in browserlist, but I think caniuse-lite doesnt have the :host feature in their feature data and also tried running querying with :host, css-host , host-selector and host selector but it will through ReferenceError: Please provide a proper feature name. Cannot find css-host.

So I don't think there is a way as of now to test it with the compatibility.

We can keep an eye on this list and when they add css-host we can write a test for this issue.

What do you think

@anikethsaha
Copy link
Member Author

Okay I guess host is in level 1 CSS Selector ?

And I think the caniuse-lite query for that is css-selection

@alexander-akait
Copy link
Member

@anikethsaha 👍

@anikethsaha
Copy link
Member Author

Closing this as this is already done. Just need to add test, I will add that later once got to know the query for level 1 selectors in caniuse-lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mergeRules merges :host(tagname) with tagname
4 participants