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

Add jshttpclient #17373

Closed
wants to merge 41 commits into from
Closed

Add jshttpclient #17373

wants to merge 41 commits into from

Conversation

juancarlospaco
Copy link
Collaborator

@juancarlospaco juancarlospaco commented Mar 14, 2021

temp

  • Unrelated CI fail:
[apt-fast 15:37:55] Working... this may take a while.
E: Unable to correct problems, you have held broken packages.
[apt-fast 15:37:55] Package manager quit with exit code.

@juancarlospaco juancarlospaco marked this pull request as ready for review March 14, 2021 16:33
changelog.md Outdated Show resolved Hide resolved


runnableExamples("-d:nimExperimentalJsfetch -r:off"):
import std/asyncjs
Copy link
Member

@timotheecour timotheecour Mar 23, 2021

Choose a reason for hiding this comment

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

this example doesn't work in a browser

I have a WIP PR to make nim doc -b:js -r --browser main which will simplify testing but until it's merged, these things must be tested manually

@Araq
Copy link
Member

Araq commented Apr 9, 2021

You can always use backticks around "method" so IMHO it should remain "method" and not some bastardized spelling of it.

@juancarlospaco
Copy link
Collaborator Author

Unrelated CI Fail with "No Errors, No Warnings" 🤷

ci_fail

is not failing by timeout.

@ringabout
Copy link
Member

ringabout commented Apr 18, 2021

It should work now if you fetch the latest commits? Unrelated CI failure should be fixed.

see #17752

@juancarlospaco juancarlospaco marked this pull request as ready for review April 18, 2021 20:45
@juancarlospaco
Copy link
Collaborator Author

@xflywind Great!, thanks, will do. If you want to take a look at the code would be nice too. 🙂

@juancarlospaco
Copy link
Collaborator Author

Ping @timotheecour

@Varriount
Copy link
Contributor

Does a user have to explicitly use the JS version of the HTTPClient, or is it done automatically depending on the target platform?

@juancarlospaco
Copy link
Collaborator Author

Does a user have to explicitly use the JS version of the HTTPClient

For native targets you import httpclient.
For JS you import jshttpclient.
An httpclient for the browser :)

@Varriount
Copy link
Contributor

Does a user have to explicitly use the JS version of the HTTPClient

For native targets you import httpclient.
For JS you import jshttpclient.
An httpclient for the browser :)

Is there any reason the implementation can't or shouldn't automatically adapt to the environment being targeted? Using completely different types makes it difficult to write generic functions that can be used across library implementations.

@juancarlospaco
Copy link
Collaborator Author

juancarlospaco commented May 3, 2021

Ye, the sandbox of the browser.
I do not want JavaScript code in the native HttpClient.
See top post that this was literally requested on the original RFC/PR.

self.send(body = body.cstring)
self.responseText

proc getContent*(self: JsHttpClient; url: Uri | string): cstring =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that this API is completely new, should we only allow Uri and not string ?.

@Araq
Copy link
Member

Araq commented Aug 11, 2021

Please contribute to https://github.com/nim-lang/standardjs instead. (Yes, it still needs CI support, will be added soon'ish.)

@Araq Araq closed this Aug 11, 2021
@juancarlospaco juancarlospaco deleted the jshttpclient branch August 11, 2021 11:41
@timotheecour timotheecour added the stale Staled PR/issues; remove the label after fixing them label Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants