Core: Warn against using jQuery.parseJSON, remove old hacks around this method

[mgol]

Fixes #150
Refs jquery/jquery#2800
Refs #152

[mention-bot]

By analyzing the blame information on this pull request, we identified @dmethvin, @markelog and @gibson042 to be potential reviewers

[dmethvin]

@markelog should I be able to see the coverage map? Coveralls can't find the source.

[markelog]

should I be able to see the coverage map?

You mean html representation of it? If yes, then i don't think so, coveralls doesn't provide such services to my knowledge.

Only

Coverage decreased (-0.1%) to 85.948%

i.e. https://coveralls.io/builds/5209155

Coveralls can't find the source.

Not sure if i understand the question

[markelog]

btw, we can provide a hook which would either warn or forbid to commit if coverage decreased.

[markelog]

Although it seems as overdoing it, since coveralls would fail the pull anyway.

[dmethvin]

With istanbul there's an HTML file showing what was covered and what wasn't. That way it is possible to improve the coverage. Having only the top-line number is not that useful since it is hard to see from that what isn't being covered. In this case it is saying coverage went down and I am not sure from the diff exactly how that happened. It appears that coveralls may be able to do the same thing as istanbul. However if you click the link for /dist/jquery-migrate.js it says the source is not available. https://coveralls.io/builds/5209155/source?filename=dist%2Fjquery-migrate.js

[markelog]

Oh, that is so cool! Didn't know about this feature, will figure this out.

[dmethvin]

Here is a shot of the last report from istanbul. Missed lines are in red and missed conditionals are in yellow. Really helpful for improving the unit tests or understanding why they are low.

[markelog]

Yeah, so that doesn't show up, cause coveralls pulls down files from the repo, but we run tests against builded version, which is obviously is not in the repo.

[dmethvin]

I am wondering how to do this. The most common reason for parseJSON issues seems to have been servers returning Content-Type: application/x-json for an empty string which we then tried to parse and rightly died since it wasn't JSON. The new AJAX code uses JSON.parse() directly which will bring back that error. In any case, jQuery.parseJSON() was defined to return null on falsy arguments so I think we still have to have the !json check there even if we don't give an additional warning about it.

[jaubourg]

There's always the possibility of being more restrictive in the regexp that detects the content-type.

[jaubourg]

Or better yet, handle empty responses at the ajax level by not having the converter be $.parseJSON directly but a wrapper around it that detects falsy values. Keeping it 100% compatible.

[dmethvin]

I was thinking that would be the best approach. That way the ajax conversion could give a specific warning about an empty string without a spurious warning due to the internal use of parseJSON.

[mgol]

Well, if Migrate 3 is supposed to bring back only deprecated/removed behavior that was present in jQuery 2.2, then jQuery.parseJSON in this version is basically JSON.parse (with one workaround for Android 2.3 but jQuery 3.0 doesn't support it in that capacity so it doesn't matter) and that's the same hook that's used by the ajax json converter. So it doesn't seem that we should continue to patch it in Migrate 3.

[jaubourg]

A good point.

[mgol]

Continuing that train of thought (and related to #152), it would seem the only workarounds/warnings that were present in Migrate 1.x and should still be present in Migrate 3 should be those that concern APIs deprecated before jQuery 1.12/2.2 but not removed in jQuery 1.12/2.2. For the rest, if someone is upgrading from jQuery 1.12/2.2 to jQuery 3.0, they already have them fixed (or their site is already broken).

[dmethvin]

I see your point, for an ajax call, it's just that in this case the potential cause of the problem is on the server and outside the user's own code. That has been one of the most commonly reported issues since we changed it in 1.9. It should be possible for someone to resolve it on the client side via explicitly stating the dataType for example, but it would be better to give a warning about that than to throw a cryptic error on the console. Our cost of doing that is small here so I don't mind improving the way the problem is reported.

For parseJSON itself we definitely need the check to be restored so that it won't die on an empty string.

[mgol]

I understand that a lot of people have this problem & Migrate helps here but those people are not running jQuery 1.12/2.2 without a Migrate and I assume Migrate 3 only plans to targets those users. Even if they had this issue, they had to already have it resolved. Moving from jQuery 2.2 without Migrate to jQuery 3.0 with Migrate 3.0 that still contains this workaround would only introduce a potential for new bugs for them if they already have logic that handles invalid AJAX requests (such as ones with a JSON content type but empty body) that would suddenly start being valid.

[dmethvin]

Per the discussion in today's meeting, this is fine. The parseJSON is the same as jQuery 2.2 and the only problem was with AJAX and should have been fixed by people before they get to Migrate 3.0.

So, 👍

[mgol]

The coverage decreased by -0.1%, lol. In any case, this is because I removed the old well-tested jQuery.parseJSON patch so I'll ignore this error.