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

#2471: Enable jQuery selectors everywhere #2499

Merged
merged 3 commits into from Jan 27, 2022

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Jan 25, 2022

Demo with the selector: h2:lt(1000)

Screen.Recording.1.mov

return;
}

isMatchinInProgress = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like there's infinite loop issue

Found it:

https://github.com/jquery/sizzle/blob/20390f05731af380833b5aa805db97de0b91268a/src/sizzle.js#L344

In some occasions Sizzle will add an id to limit the scope of the selector. I triggered it with h2:not([id]) ~ h2

This is the suggested workaround and the browser no longer hangs.

Choose a reason for hiding this comment

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

It looks like you found the issue where the browser was hanging using jQuery selectors? It was a bug with Sizzle? Is this something that has been fixed upstream? If so, do you think it might be safe to include this in jQuery.initialize?

Choose a reason for hiding this comment

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

Possibly related jquery/jquery/issues/4332?

@twschiller
Copy link
Contributor

Thanks! - good find on the selectors.

Reading the PR, I realized that we had already implemented polling for initialize. So we could have just removed the isNativeSelector checks from triggerExtension b/c they should have worked?

This new PR has different performance characteristics than the old method, as it now has to watch the document for all mutations. I need to dig in a bit more here to see what the tradeoffs are

@fregante
Copy link
Collaborator Author

fregante commented Jan 25, 2022

Unless I made a mistake, the method/scope of the watcher hasn't changed. The only thing that changed was jQuery selectors are no longer polled but observed.

It would not have just worked because initialize uses querySelectorAll. Maybe I'm misunderstanding what you're saying. It’s also possible that you’re misreading the original code like I did in my now-hidden comment 😅

@fregante fregante marked this pull request as ready for review January 25, 2022 15:33
@twschiller
Copy link
Contributor

twschiller commented Jan 25, 2022

The only thing that changed was jQuery selectors are no longer polled but observed.

The way that the vendored initialize method works is that it observes the root (document) for mutations, which it then runs the selectors over. The question is what the performance impact will be if we're running JQuery instead of native selectors. (Although, my understanding is JQuery will use native selector evaluation under the hood for native selectors"?)

For 1.5.2 (which needs to go out ASAP), I think I will just remove the assertion on "watch" mode, because our "initialize" method does actually support polling via await element. That will then give us more time to merge in this PR for 1.5.3

It’s also possible that you’re misreading the original code like I did in my now-hidden comment 😅

The Global.ts file's "initialize" refers to the vendored $.initialize method which does not support JQuery. I think it's right that we could use _initialize, but would have to be careful to re-attach it because it only fires once

I might also still be misreading the code though 🤷

@twschiller twschiller self-requested a review January 27, 2022 02:50
@twschiller twschiller enabled auto-merge (squash) January 27, 2022 03:21
@twschiller twschiller merged commit 807c143 into main Jan 27, 2022
@twschiller twschiller deleted the F/feature/jquery-selectors-everywhere branch January 27, 2022 03:23
@twschiller
Copy link
Contributor

I'm a little confused how the code loop check works as the mutation callback is synchronous. Not fully following how another mutation event could fire while one is being processed. But have tested it and it seems to work OK

@twschiller twschiller added this to the 1.5.2 milestone Jan 27, 2022
@bezborodow
Copy link

Interesting.

Possibly related jquery/jquery/issues/4332? See my comments above.

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.

Partially support non-native CSS selectors for trigger "watch" mode
3 participants