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

Update to jQuery 3.x. #772

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Update to jQuery 3.x. #772

merged 2 commits into from
Feb 7, 2018

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Feb 5, 2018

This shouldn't result in any functional change.

The main difference between jQuery 2.x and jQuery 3.x, at least as impacts GeoJS, is that Deferred objects are less synchronous. Specifically, the done, fail, and always methods can be
synchronous if the deferred is resolved or rejected, but the then method is never synchronous. The major changes are in the tests which relied on the synchronicity for running the tests; they no longer do so, releasing time slices as appropriate.

Since GeoJS objects could take native promises, there is code to use always if available and then if not. In other places, we rely on jQuery's Deferred having the done and always convenience functions.

This shouldn't result in any functional change.

The main difference between jQuery 2.x and jQuery 3.x, at least as
impacts GeoJS, is that `Deferred` objects are less synchronous.
Specifically, the `done`, `fail`, and `always` methods can be
synchronous if the deferred is resolved or rejected, but the `then`
method is never synchronous.  The major changes are in the tests which
relied on the synchronicity for running the tests; they no longer do so,
releasing time slices as appropriate.

Since GeoJS `object`s could take native promises, there is code to use
`always` if available and `then` if not.  In other places, we rely on
jQuery's `Deferred` having the `done` and `always` convenience
functions.
@@ -165,8 +168,11 @@ describe('geo.transform', function () {
results: []
}));

expect(spy.calledOnce).toBe(true);
expect(geo.transform.defs.hasOwnProperty('EPSG:5001')).toBe(false);
window.setTimeout(function () { // wait for the next time slice
Copy link
Member

Choose a reason for hiding this comment

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

why we need window timeout now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jQuery 3 is always asynchronous for certain requests where jQuery 2 was sometimes synchronous. Specifically, we are making an ajax request to get the transform from a web service. We mock the web service for the test, so the mock returns immediately. But, in jQuery 3, the callback is fired in the next time slice rather than synchronously, so we wait 0 ms (just to be in the next time slice) to make sure the callback has been triggered.

In jQuery 2, the behavior was ajax->mock response->callback->next time slice.

In jQuery 3, the behavior is ajax->mock response->next time slice->callback.

If we were doing an actual request which couldn't be served from browser cache, we would have to wait an unknown amount of time before the callback would be triggered. It is only because we are mocking the response that the immediate check worked in jQuery 2 (and the check with a 0-duration wait works in jQuery 3). Rather than use window.setTimeout, we could have used time mocking, but that doesn't make the test any clearer in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the detailed explanation @manthey.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me

@manthey manthey merged commit e43b8c0 into master Feb 7, 2018
@manthey manthey deleted the update-jquery branch February 7, 2018 14:49
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.

2 participants