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 handling of jQuery objects in `TooltipManager.add` method #16144

Merged
merged 2 commits into from Nov 9, 2017

Conversation

Projects
None yet
5 participants
@jasonrudolph
Member

jasonrudolph commented Nov 9, 2017

Background

In #15972, I converted the TooltipManager class from CoffeeScript to JavaScript using decaffeinate:

As part of that conversion (7b76ee3), this line of CoffeeScript:

disposable.add @add(element, options) for element in target

Became this line of JavaScript:

for (let element of target) { disposable.add(this.add(element, options)) }

That change led to the regression described in #16135.

A for...of loop requires that the given object implement the iterable protocol. As @UziTech points out in #16135 (comment), jQuery objects gained support for the iterable protocol in jQuery 2.2.0, but earlier versions didn't support it. Since many Atom packages use space-pen for their views, and since space-pen uses jQuery 2.1.4, the change in #15972 introduced a regression impacting those packages.

Proposed solution

To address the regression, this PR does two things:

  1. fb74992 enhances the existing test that exercises the handling of jQuery objects in TooltipManager.add. By using a fake jQuery object that more closely matches a real jQuery object, the test fails with the same error seen in #16135: https://circleci.com/gh/atom/atom/6007
  2. 0e82b8b replaces the for...of loop with a traditional for loop, and that fixes the test failure and resolves the bug.

Proposed test plan

In addition to ensuring that the test suite is passing, I used the following steps to manually verify that this change fixes the bug:

If there's any additional verification that would be helpful here, please let me know.

jasonrudolph added some commits Nov 8, 2017

Enhance test to catch bug reported in #16135
Enhance the fake jQuery object to more closely match a real jQuery
object. With this change, the test fails, thus allowing us to reproduce
the regression reported in #16135.

@jasonrudolph jasonrudolph requested a review from maxbrunsfeld Nov 9, 2017

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Nov 9, 2017

Contributor

👍

Contributor

maxbrunsfeld commented Nov 9, 2017

👍

@jasonrudolph jasonrudolph merged commit dd8173f into master Nov 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonrudolph jasonrudolph deleted the fix-jquery-handling-in-tooltip-manager-add-method branch Nov 9, 2017

jasonrudolph added a commit that referenced this pull request Nov 9, 2017

Merge pull request #16144 from atom/fix-jquery-handling-in-tooltip-ma…
…nager-add-method

Fix handling of jQuery objects in `TooltipManager.add` method
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 9, 2017

Contributor

👍

Contributor

nathansobo commented Nov 9, 2017

👍

@youyinnn

This comment has been minimized.

Show comment
Hide comment
@youyinnn

youyinnn Nov 11, 2017

still don't know how to fix it

youyinnn commented Nov 11, 2017

still don't know how to fix it

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Nov 11, 2017

Contributor

@youyinnn it will be fixed in the next beta release

Contributor

UziTech commented Nov 11, 2017

@youyinnn it will be fixed in the next beta release

@youyinnn

This comment has been minimized.

Show comment
Hide comment
@youyinnn

youyinnn Nov 11, 2017

@UziTech thx for the reply

youyinnn commented Nov 11, 2017

@UziTech thx for the reply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment