Stop core clone() from reaching for a global jQuery/Zepto - #2476
Open
afonsojanu wants to merge 1 commit into
Open
Stop core clone() from reaching for a global jQuery/Zepto#2476afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
clone() checked window.jQuery and window.Zepto and used their clone method whenever either happened to exist on the page, regardless of whether jquery-sortablejs was actually the thing driving Sortable. That means any page with jQuery loaded for unrelated reasons, but using plain SortableJS directly, silently got jQuery's clone instead of a native cloneNode, which breaks in interesting ways if that jQuery-like global doesn't behave exactly like real jQuery. Checked jquery-sortablejs itself: it never touches this function or relies on it reaching for jQuery internally, it just wraps Sortable in a $.fn plugin. So core has no real reason to special-case jQuery here at all, native cloneNode is what it should use unless something more specific (Polymer, in this file) says otherwise. Fixes SortableJSGH-2453.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2453.
clone() in src/utils.js checked window.jQuery and window.Zepto and used their clone() whenever either happened to be present on the page, no matter whether jquery-sortablejs was the thing actually driving the widget. So a page that loads jQuery for something completely unrelated, but uses plain SortableJS directly, would silently get routed through jQuery's clone instead of a native cloneNode. The linked issue shows exactly this: a non-jQuery global that merely calls itself jQuery breaks the whole thing since it doesn't implement clone() the way real jQuery does.
I went and checked the jquery-sortablejs package itself to make sure removing this wouldn't quietly break it: it's a thin $.fn wrapper around Sortable, it never calls into or depends on this clone() function reaching for jQuery internally. So there's no real coupling here, core doesn't need the jQuery/Zepto branch at all, and cloneNode is the right default the same way it already is when neither is present.
I couldn't run the existing TestCafe suite in my environment (no screen-recording permission on this machine, which TestCafe's browser-tools needs even in headless mode on macOS), so instead I wrote a small standalone Node script that loads the built UMD bundle with a stubbed window/document and a fake jQuery-like global matching the issue's repro, and confirmed the old code throws trying to call the fake global's clone(), while the patched version correctly falls through to cloneNode. Happy to share that script if useful, just didn't want to add throwaway test infra to the PR itself.