dperini / nwsapi

Fast CSS Selectors API Engine
MIT License
103 stars 35 forks source link

Revert to v2.2.2 #104

Open asamuzaK opened 6 months ago

asamuzaK commented 6 months ago

Most of the issue reports over the past year, e.g. #82, #83, #88, #89, #90, #95, #99, #100, #103 etc. indicate that the regular expressions used in v2.2.3 and later are incorrect. I suggest reverting to v2.2.2 once and review the regexp.

asamuzaK commented 6 months ago

As far as I tested, nwsapi does not handle the followings correctly.

dperini commented 6 months ago

@asamuzaK you are welcome to suggest changes/improvements that you see fits. All of the above problems are already listed in the issues. I am going to commit attempts to fixes for all of them.

dperini commented 4 months ago

@asamuzaK a few fixes landed in "nwsapi" and some are related to the problems you listed. Can you go through the faulty queries and see if the validator logic is now working ? Thank you for your help.

asamuzaK commented 4 months ago

Could you please publish the pre-release version to npm? Then I think I can help you with the test.

dperini commented 4 months ago

Do you mean publish a 2.2.10 release with the latest changes in github master or something else ?

I was already planning a release for tomorrow afternoon and I would prefer that if you can wait just a few more hours.

Validator problem is fixed and is the next fix I would commit on github later today, the problem was brought up by recent use of pseudo-classes names containing dashes (those should be escaped).

A meaningful release is planned for tomorrow after some clean up and testing of the fixes for focus-xxx, dir(rtl/ltr), media states and remaining open issues. Many of the issue will be cleared by the validator fixes.

I am sorry for the hassle this created in various projects, but I will do my best to have everything running smooth again with due additions and improvements with a new way/tool to test my work based on WPT testing suite that would give extra testing capabilities to have users help me in the maintenance of "nwsapi".

New commits should start flowing in a few hours for you to review and suggest.

Thank you in advance for your help and contributions.

asamuzaK commented 4 months ago

I was already planning a release for tomorrow afternoon and I would prefer that if you can wait just a few more hours.

Great, I'll wait for it.

As a side note, I recommend you to write unit tests for each commit.

dperini commented 4 months ago

@asamuzaK I have started the commits I was talking about. Maybe you can give a look to the validator and suggest improvements. Will continue to do commits to fix bugs and add new capabilities in Selectors Level 4.

asamuzaK commented 4 months ago

Some feedback. I will explain with some logs based on jsdom+nwsapi.

First is the log of v2.2.2. jsdom_nwsapi2_2_2.txt There are 2 errors compared to v2.2.7. The first error is related to :focus, but this is a bug in jsdom itself, so it is expected to throw an error. jsdom does not lose focus and continues to maintain focus even if the attr of the focused element changes and the element is expected to lose focus. In other words, v2.2.2 has the correct implementation regarding :focus. The second error has the message Hey, did you fix a bug?. The expected error did not occur and it means that there is a regression between v2.2.2 and v2.2.7. It shows that the implementation of :is(), :where() added after v2.2.2 is incorrect.

Next is a comparison between v2.2.7 and v2.2.9. jsdom_nwsapi2_2_9.txt There are 12 errors. 8 of them are Hey, did you fix a bug?; validation errors related to :has() did not occur. But please note that the implementation of :has() itself has not been resolved. The 9th error is related to :scope, this is a step forward. Great work. The remaining 3 errors are regressions related to namespace and CSS ident-token, that means the implementation in v2.2.7 was much more reasonable.

And the current master (commit 720c5e8) jsdom_nwsap_master_720c5e8.txt All 8 errors are regressions.

From above, I have to say that it has gotten worse and worse with each version since v2.2.2. I strongly recommend to revert all of the commits since v2.2.9.

I'm planning to cut a branch from v2.2.9 and help to fix below.

dperini commented 3 months ago

@asamuzaK now the logical selectors regular expression is the one used in 2.2.2 and many things are fixed. Is it worth a 2.2.10 release or should we wait for more fixes ?

asamuzaK commented 3 months ago

Maybe it's because of my poor English, I'm frustrated with myself that I can't communicate well with you...

The code in current nwsapi's master branch has too many regressions. No matter how much you try to modify the regular expressions from the master, there will be few improvements. It's better to just go back to some point i.e. v2.2.2.

For example, see Review source by asamuzaK · Pull Request #113 · dperini/nwsapi It is a new branch cut from v2.2.2 and added some modifications. Main difference from the master is that it lacks :has() and focus-visible supports at the moment, but no regressions. To test this PR:

dperini commented 3 months ago

@asamuzaK I can fully understand what you say and I wish to thank you for the help. I normally execute the tests as you describe using jsdom, but I also execute all the tests I can in wpt and all the ones in the /test directory of my own repository.

If you want I can talk in a conference call through Skype, I have it currently open and my alias name there is "nwbox_tech".

It will be helpful to have a chat, writing is not as powerful in this case (due to a problem of mine). But we are good to go no more errors that I see locally (minor errors exists but are not urgent like the blocker I have been fixing.

asamuzaK commented 3 months ago

I told you that there are many regressions in the changes made after 2.2.9, so you should revert it. I do not recommend releasing it as 2.2.10.

If you want I can talk in a conference call through Skype,

No thank you.

dperini commented 3 months ago

@asamuzaK everything is back to normal on my side, late though but that's how I can do it (slowly). Going back to 2.2.2 is not an option for me, I solved all the necessary bits and in the afternoon I plan to release 2.2.10 ... only test failing is the ":has(> :scope)" mangled selector which I never supported anyway. Also ":has()" is getting tentative support and recently a bug was fixed.

asamuzaK commented 3 months ago

Ref #115 As I said, there are a lot of regressions.

cee-chen commented 3 weeks ago

only test failing is the ":has(> :scope)" mangled selector which I never supported anyway. Also ":has()" is getting tentative support and recently a bug was fixed.

:has() is now fully supported by all evergreen browsers - is there any update on when nwsapi will support the selector more fully? For context, we've had to pin to 2.2.2 to prevent test failures on the following selector, which works perfectly in browsers:

*:has(*:is(.classA, .classB)) + *:is(.classC, .classD) {
  color: red;
}

interestingly, the above works fine without the second :is() and also works fine with :not() instead of :has().

edit: The following previous sibling selector also throws nwsapi errors:

&:has(+ .classA) {
  color: red;
}