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

2.1.2-pre, ajax.js: environments that don't have 'location' variable will break #1788

Closed
mgol opened this issue Oct 21, 2014 · 17 comments
Assignees
Labels
Milestone

Comments

@mgol
Copy link
Member

@mgol mgol commented Oct 21, 2014

Originally reported by brianblocker at: http://bugs.jquery.com/ticket/15210

It appears that the change which removes the "IE workaround" on ajax.js now breaks environments that do not have window.location. To be specific:

(from  https://github.com/jquery/jquery/blob/master/src/ajax.js#L44) ajaxLocation = location.href,

When I try to run automated tests using jasmine-node, location is undefined (because there is no "browser" sorta), and therefore the test breaks. Using the previous version (2.1.1-pre) works fine, but it appears that the change introduced in 2.1.2-pre now breaks the environment.

Issue reported for jQuery 2.1.1

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Oct 21, 2014

Comment author: dmethvin

Since this is a regression, I'd like to fix it. However, we don't unit test or support jasmine-node so to have any chance to do this we'll need your help. Can you propose a fix as a pull request?

@mgol mgol added the Bug label Oct 21, 2014
@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014
@mgol mgol self-assigned this Oct 21, 2014
@markelog markelog added the Ajax label Nov 2, 2014
@mgol mgol added the 2.x-only label Nov 6, 2014
@denis-sokolov

This comment has been minimized.

Copy link

@denis-sokolov denis-sokolov commented Dec 17, 2014

This is now a regression in a patch-level release, it broke all our headless testing.

I see the added milestone 3.0.0 and it alarms me.
Could you please clarify me your position on supporting non-browser environments?
The support is still in README and in intro.js.

@dmethvin

This comment has been minimized.

Copy link
Member

@dmethvin dmethvin commented Dec 17, 2014

It shouldn't have been landed in 2.1.2. I'll investigate.

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Dec 17, 2014

@denis-sokolov could you write how exactly you are requiring jQuery in your code?

@dmethvin

This comment has been minimized.

Copy link
Member

@dmethvin dmethvin commented Dec 17, 2014

@denis-sokolov it had been assigned to 3.0 and only a small number of things were pulled back for this 2.1.2 release. We expect 3.0 to be out in a month or two so it's not going to be a long wait. We also need to find a way to test this to prevent regressions, which is the root cause of the problem in the first place.

@denis-sokolov

This comment has been minimized.

Copy link

@denis-sokolov denis-sokolov commented Dec 17, 2014

Run jQuery in Node, it's simple:

var jqueryFactory = require('jquery');
var doc = jsdom.jsdom(html);
var w = doc.parentWindow;
var $ = jqueryFactory(w);
@dmethvin

This comment has been minimized.

Copy link
Member

@dmethvin dmethvin commented Dec 17, 2014

The best way to be sure this lands is to create a pull request. We do want it to happen.

@denis-sokolov

This comment has been minimized.

Copy link

@denis-sokolov denis-sokolov commented Dec 17, 2014

@mzgol has stated that you won't release a new patch quite quickly. I understand the release process is complex in a case of a big project like jQuery and I make no judgement on your decision.

I want to make sure that you have the full information, however.
The way I see it, currently jQuery is completely broken on Node.js and everyone needs to blacklist version 2.1.2 in their package.json to fix their build. It's inconvenient.

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Dec 17, 2014

everyone needs to blacklist version 2.1.2 in their package.json

Sorry, I assumed for a while that it's something that we broke in 2.1.1 and just didn't fix in 2.1.2 but now I see it was working correctly in 2.1.1: http://code.jquery.com/jquery-2.1.1.js. It was working a little by accident as in Node with jsdom the location.href check was throwing but there was a try-catch around it that was fixing it. This makes the problem more serious than I thought.

@denis-sokolov can you confirm that changing just this one line fixes the problem?

@markelog

This comment has been minimized.

Copy link
Member

@markelog markelog commented Dec 17, 2014

This is the kind of thing i fear would happen, i think we should start running smoke tests on environments like this, i.e. jsdom, phantom, etc;

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Dec 17, 2014

Running the Promises/A+ tests in Node (see e1303a3) will fix this for Node but that's still the future.

@denis-sokolov

This comment has been minimized.

Copy link

@denis-sokolov denis-sokolov commented Dec 17, 2014

@mzgol, yes, after I changed that line, ran grunt, and copied dist/jquery.js to my project, all my tests passed. I confirmed with this patch on top of master and on top of 2.1.2.

@markelog

This comment has been minimized.

Copy link
Member

@markelog markelog commented Dec 17, 2014

Yeah, although this is not an original intentent of it and does not cover other environments

@dmethvin

This comment has been minimized.

Copy link
Member

@dmethvin dmethvin commented Dec 17, 2014

Agree that we need smoke tests for these environments, that the Promises/A+ tests will help there for node, and that we need to enable JSHint options to disable all but window and document if possible so we don't accidentally use them. We're planning to have 3.0 out in January and it can address at least some of these.

@denis-sokolov there's a lot of work involved in pushing a release including sending it to the CDN providers so I would prefer not to do that all over again. It was a 24-hour process to get here because of various issues with the process, not all of which are fixed and some of which require manual steps.

I don't think we have a lot of node users at the moment and you can set your dependency specifically to 2.1.1 for now. Sorry. How exactly are you using jQuery on the node side?

@denis-sokolov

This comment has been minimized.

Copy link

@denis-sokolov denis-sokolov commented Dec 17, 2014

@dmethvin, I run automated tests and automated measurements on code that in production is run in browsers. jsdom provides a much more convenient and fast environment than Phantom.js.

I also am preparing to use my environment-agnostic modules to analyze HTML documents in a pure server-side application.

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Dec 17, 2014

OK; PR #1949 should serve as a basic smoke-test for non-browser environments; it still requires work, though.

@mgol

This comment has been minimized.

Copy link
Member Author

@mgol mgol commented Dec 18, 2014

For reference, this was fixed for 2.1-stable in 685fae9; smoke-testing added in a followup 998556f.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.