Skip to content

timeout setting for Request #151

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

Merged
merged 2 commits into from
Sep 6, 2020

Conversation

chekoopa
Copy link
Contributor

@chekoopa chekoopa commented Sep 4, 2020

A simple timeout feature with Maybe field in Request. Test case included. Closes #143.

It was easier than I've thought.

@chekoopa
Copy link
Contributor Author

chekoopa commented Sep 4, 2020

Had to force-push twice because of JS formatting fixes. Srry.
But still has strange issues with foreign code, however the test has been successfully completed on the local machine with Spago 0.16.0 and Purs 0.13.8.

@thomashoneyman
Copy link
Contributor

@chekoopa Libraries in the purescript-contrib organization only support ES5 in the FFI to ensure libraries can be used without a bundling step. I'm not sure if that's specifically the error you're seeing, but I do see const and let in there, which will parse with 0.13.8 but isn't ES5.

Do you mind switching over to use ES5 in the FFI file? Most likely that will also resolve the parse issue with the foreign JS as well. In this case it mostly looks like that comes down to swapping var for const / let.

@chekoopa
Copy link
Contributor Author

chekoopa commented Sep 4, 2020

@thomashoneyman Sure, done that, let's see what Travis would say.

@thomashoneyman
Copy link
Contributor

Checked the documentation and looks like setting 0 is the correct setting for "no timeout", so the Maybe encoding looks appropriate.

@chekoopa
Copy link
Contributor Author

chekoopa commented Sep 4, 2020

Checked the documentation and looks like setting 0 is the correct setting for "no timeout", so the Maybe encoding looks appropriate.

Yes, I've done it first without Maybe, but '0 ms' as a default value may create some misconceptions.

Copy link
Contributor

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chekoopa. I'd like to give @garyb a chance to take a look, but I think this looks good.

@thomashoneyman thomashoneyman merged commit 8e1bd69 into purescript-contrib:master Sep 6, 2020
@garyb
Copy link
Member

garyb commented Sep 6, 2020

I think this should have been released as a breaking change - adding the field to Request means it will break for anyone who is constructing the value manually rather than overriding defaultRequest.

@thomashoneyman
Copy link
Contributor

🤕 don't release late at night. I've updated the release to be v11.0.0 as this didn't yet make it into Spago and it's only been a few hours.

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

Successfully merging this pull request may close these issues.

Provide an option for configuring xhr.timeout
3 participants