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

fix xhr.response.body.text is not a function #1

Merged
merged 4 commits into from
Feb 12, 2020
Merged

fix xhr.response.body.text is not a function #1

merged 4 commits into from
Feb 12, 2020

Conversation

zhex900
Copy link
Contributor

@zhex900 zhex900 commented Feb 11, 2020

This a fix for this error.

Screen Shot 2020-02-12 at 8 34 56 am

@archfz
Copy link
Owner

archfz commented Feb 11, 2020

In what case those this happen? Is it when the request has empty body?

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 11, 2020

I guess so.

@archfz
Copy link
Owner

archfz commented Feb 11, 2020

Can you make a modification in order to still log the request but without body in case the .text method does not exist? We still would like to log the request since it is important information.

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 11, 2020

Can I use await ?

Don't worry I added async

if(xhr.response.body) {
          const response = typeof xhr.response.body.text === 'function' ? await xhr.response.body.text(): 'EMPTY BODY';
          
          logs.push([String(xhr.status).match(/^2[0-9]+$/) ? 'cy:route:info' : 'cy:route:warn',
            `Status: ${xhr.status} (${route.alias})\n\t\tMethod: ${xhr.method}\n\t\tUrl: ${xhr.url}\n\t\tResponse: ${response}`]);

      }        

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 11, 2020

I have updated my PR.

@archfz
Copy link
Owner

archfz commented Feb 12, 2020

@zhex900 I have been testing the change and the thing is like so: The response.body will be a stream in case it's the new FETCH API used. In case the old XMLHTTPREQUEST API is used then it will be either a text or a de-serialized object.

You case is XMLHTTPREQUEST and that's why the text() method never exists on the body.

So this means that we need a new change here:

  1. First check if the body evaluates to true -> if not use the EMPTY_BODY string
  2. Second check if the body is typeof string -> if yes use it as is
  3. Third check if the body is object and has the method text() -> if yes use text() to get the content
  4. Last check if the body is object -> use JSON.stringify() on it to get the content
  5. Else set the body to some string, like UNKNOWN_BODY or something.

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 12, 2020

Screen Shot 2020-02-13 at 8 14 52 am

Please have a look at my update. Can I add a .prettierrc.js ?

module.exports = {
  trailingComma: "es5",
  tabWidth: 2,
  singleQuote: true,
  printWidth: 100,
  bracketSpacing: false,
};

@archfz
Copy link
Owner

archfz commented Feb 12, 2020

Yes of course. I am now testing the changes. After you've added it I will merge it and tomorrow create a release with some tests as well. This is quite an important bug. Thanks for your contribution.

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 12, 2020

Do you prefer using double quotes or single quotes?

@archfz
Copy link
Owner

archfz commented Feb 12, 2020

I will leave the formatting to you since I do not have a preference. 😄

@zhex900
Copy link
Contributor Author

zhex900 commented Feb 12, 2020

On a separate note, I want to see the logs in the terminal even if the tests passes. How do you feel about adding an extra config option?

@archfz
Copy link
Owner

archfz commented Feb 12, 2020

Yeah sure, but let's keep this MR like so. If you'd like the feature you can create the feature request or if you want to do it yourself just create a new MR. Also I have tested your changes and they are working well, good job.

@archfz archfz merged commit 26d19d8 into archfz:master Feb 12, 2020
archfz pushed a commit that referenced this pull request Jun 16, 2020
archfz pushed a commit that referenced this pull request Apr 15, 2022
archfz pushed a commit that referenced this pull request Jan 29, 2023
Update to latest, merge from base repo
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.

3 participants