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

Fixing the XHR bug where the json is assumed to be a string in IE #4641

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsundquist
Copy link
Contributor

Description of changes

Updates all XHR request that are trying to request json data to include the option of json: true. Without this option IE treats the responses as a string.

Related issues (if any)

Fixes #3734

Testing

  • Please confirm npm run test-all ran successfully.

@autoboxer
Copy link
Contributor

autoboxer commented May 25, 2018

I'm concerned about the following comment by @AurelioDeRosa in the original issue about the proposed fix using json: true:

I've done some further research and the solution proposed isn't viable. When json is set to true, it expects a serializable object to be set as the value of the body option. The CMS sends a FormData object which is not serializable.

@JedWatson or @Noviny, would you mind weighing in on this issue?

One note: I'm wondering if the RelationshipField.js and RelationshipFilter.js files need to be considered in this fix @jsundquist, as they also use responseType: 'json'.


I'm pasting this here for more visibility as it's a workaround, and is a copy of a comment by @AurelioDeRosa from issue #3734:

An easy workaround for accessing the items only (e.g. no add or edit) is to add the following snippet:

if (typeof data === 'string') {
   data = JSON.parse(data);
}

to all the XHR requests that return a JSON file. Specifically, those are the calls that have responseType: 'json'. They can be find in the following files:

keystone/admin/client/utils/List.js
keystone/fields/types/relationship/RelationshipField.js
keystone/fields/types/relationship/RelationshipFilter.js

In general the snippet must go before data is used in the callback of the Ajax call.

@stennie
Copy link
Contributor

stennie commented Jun 14, 2018

@jsundquist Is this fix also required for Microsoft Edge? Can you confirm the full build version of IE you are testing with (and Edge, if available)?

Thanks,
Stennie

@jsundquist
Copy link
Contributor Author

Testing was done within IE11 only, (11.0.9600.19002). Unfortunately I do not believe that I have access to Edge at this time.

Based on the comments above as well by @autoboxer referencing the research that @AurelioDeRosa did within issue keystonejs/keystone#3734 (comment) this may need a second look.

@cantuket
Copy link

May not be the most elegant, but I think implementing an interim revision of @AurelioDeRosa solution (though we definitely need to be validating the result of JSON.parse()) as a small utility function would be safest for now...

const checkJSON = data => {
  if (typeof data !== 'string') return data;     // not a string, just return it
  let isJSON = JSON.parse(data);  
  if (isJSON) return isJSON;                    // its JSON, so return parsed
  return data;                                  // just little old string
});

while one of the core developers makes a decision about replacing 'xhr' with 'fetch', 'axios' etc., which seem to handle this more gracefully.

I'm happy to implement and submit a PR, but I can't say with any certainty that I'll find all the areas this is needed.

@cantuket
Copy link

However unlikely, all these cases could be an issue though...

JSON.parse('false')
// false
JSON.parse('null')
// null
JSON.parse('true')
// true
JSON.parse('[]')
// []
JSON.parse('{}')
// {}

Here's my edits, but I'm sure some other scenarios need to be handled

const checkJSON = data => {
   if (typeof data !== 'string' || data === 'false' || data === 'true' || data === 'null') return data;
  let isJSON = JSON.parse(data);  
  if (isJSON) return isJSON;
  return data;                                  
});

Or we can just leave all this type checking up to someone else...
https://www.npmjs.com/package/is-json

import * as isJSON from 'is-json';

const checkJSON = data => {
  if (typeof data !== 'string') return data;
  if ( isJSON.strict(data) ) return JSON.parse(data);
  return data;                                  
});

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

Successfully merging this pull request may close these issues.

4 participants