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

request library fix #14

Merged
merged 2 commits into from
May 23, 2023
Merged

request library fix #14

merged 2 commits into from
May 23, 2023

Conversation

freyamade
Copy link
Contributor

Ran into an issue while updating my website for 6.4, had to update the xivapi lib and was getting this weird issue.

The port to @root/request, from what I can tell, missed a semi-important change, as it expects url and not uri to be passed. I don't know if uri was used in prior versions, I wasn't able to test it because it would crash due to some other issue so I figured the best thing to do would be to fix it and bump the requests library to the version that is currently working for me.

@kaimoe
Copy link
Member

kaimoe commented May 15, 2023

I just checked manually and the current setup works just fine as it always did, both with the minimum required @root/request version and the latest one. The uri/url key synonymy hasn't changed for request and, as far as I can tell from glancing at its code, a fully-qualified URI like the one we use (https://xivapi.com/path) gets the same treatment as using a parsed URL.

Capture

Given this, it's really weird to be getting different different results from the change you made. Could you give any insight into your current setup (node/react/something else? versions? specific methods where the error happens?) or the error output to help me make some sense of this?

@freyamade
Copy link
Contributor Author

Hey! No problem, happy to try and problem solve! I'm getting this problem from a vue component trying to just call the following;

const xiv = new XIVAPI()
    try {
      const response = await xiv.character.get(id)

When I updated the lib to 0.4.1 it installed @root/request 1.9.2. When running the code above I was getting this error;
Screenshot from 2023-05-16 02-06-17

That brought me into the request library to a line that in my opinion should be fixed in that library as well, but I can't find a repo with this version of the code anywhere so I'm not sure how to go about that.

request 1 9 2 debug info

The main thing from the image is that there is a fetch call, on line 132, using opts.url which as you can see is null. Down at the bottom of the picture is both the line causing the issue above, and in the debug info you can see the response is actually from a fetch request to the localhost dev server, which breaks the json parse and then causes the error above since .json() consumes the body before .text() tries to.

As for the previous versions, both 1.7 and 1.8 versions of the library give me this error instead

Screenshot from 2023-05-16 02-02-39
Screenshot from 2023-05-16 02-03-10

I'm not 100% sure on what this should be or why it all works properly for you and not for me but if you know something I maybe missed then hopefully that should be all I need!

@kaimoe
Copy link
Member

kaimoe commented May 16, 2023

Thanks a lot for the documentation, that helped. As I suspected, requests is showing different behavior in server and client-side operation. It seems when you include the package you get the functions from urequest.js, while I was getting request.js, confirmed from their package.json. The server-side version has a routine to ensure either url or uri work fine, but the browser version doesn't, thus giving you the empty url request that breaks.

Though it should be fine to merge your changes as-is, I've opened an issue and will wait at least a couple days for a response first just in case.

@freyamade
Copy link
Contributor Author

That's really weird.. but thank you for this, I really appreciate the help :D

@kaimoe
Copy link
Member

kaimoe commented May 23, 2023

Maintainer hasn't responded, and testing has showed no reason not to merge. New version up soon.

@kaimoe kaimoe merged commit b04858b into xivapi:master May 23, 2023
@freyamade
Copy link
Contributor Author

Thanks again so much for the help, much appreciated!

@freyamade freyamade deleted the root-request-fix branch May 23, 2023 21:58
@roncli
Copy link
Contributor

roncli commented May 24, 2023

Yo, thanks for catching this. I use this server-side, so didn't come across the same issue.

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