Skip to content

timeout setting for Request#151

Merged
thomashoneyman merged 2 commits into
purescript-contrib:masterfrom
chekoopa:xhr-timeout
Sep 6, 2020
Merged

timeout setting for Request#151
thomashoneyman merged 2 commits into
purescript-contrib:masterfrom
chekoopa:xhr-timeout

Conversation

@chekoopa

@chekoopa chekoopa commented Sep 4, 2020

Copy link
Copy Markdown
Contributor

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

It was easier than I've thought.

@chekoopa

chekoopa commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

chekoopa commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

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

@thomashoneyman

Copy link
Copy Markdown
Contributor

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

@chekoopa

chekoopa commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

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.

@thomashoneyman thomashoneyman left a comment

Copy link
Copy Markdown
Contributor

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

garyb commented Sep 6, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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