Issue found in get_url_options function in fetch.rs

[gc-victor]

There is a issue in the get_url_options function. In the current implementation, if a user uses as an input a URL or Request object, there is a "Missing required URL" error.

A fetch can receive a String, URL, or Request as an input, so when it is an object, it has to get the url string from the URL (href/toString()) or the Request objects.

https://github.com/awslabs/llrt/blob/a34b403d3b77277d86637462954ecce94fc96d7c/src/http/fetch.rs#L184

It should be something like this:

fn get_url_options(resource: Value) -> (Option<Result<String>>, Option<Object>) {
    if resource.is_string() {
        let url_string = resource.get();
        return (Some(url_string), None);
    } else if resource.is_object() {
        let resource_obj = resource.into_object().unwrap();
        let ctor: Object = resource_obj.get("constructor").unwrap();
        let name: String = ctor.get("name").unwrap();

        if name == "URL" {
            let href = resource_obj.get("href");
            return (Some(href), None);
        } else if name == "Request" {
            let url = resource_obj.get("url");
            return (Some(url), Some(resource_obj));
        }

        return (None, Some(resource_obj));
    }
    (None, None)
}

Once this is solved, there is another bug in this line:

https://github.com/awslabs/llrt/blob/a34b403d3b77277d86637462954ecce94fc96d7c/src/http/fetch.rs#L108

A mapping has to be done between the two objects.

In the MDN documentation, there is a link to an example of how it could work:

const myOptions = {
    method: "GET",
    headers: myHeaders,
    mode: "cors",
    cache: "default",
};

const myRequest = new Request("flowers.jpg");

fetch(myRequest, myOptions)

https://github.com/mdn/dom-examples/blob/6d7115ef281a5576d5cc53532380e845eb1adc07/fetch/fetch-with-init-then-request/index.html

Reference: https://developer.mozilla.org/en-US/docs/Web/API/fetch#resource https://github.com/awslabs/llrt/blob/a34b403d3b77277d86637462954ecce94fc96d7c/src/http/fetch.rs

[richarddavison]

Thanks for that crystal clear bug report @gc-victor! I'm working on a fix!

[gc-victor]

@richarddavison if you didn't start it, I can work on it.

[richarddavison]

@gc-victor thanks but almost ready with a PR 🥇

[richarddavison]

I think this should cover it: https://github.com/awslabs/llrt/pull/327 Tries to get option from options arg first. Then if not found falls back to "resource" property. If resource property is is not Request and has a toString method (for example URL) that one is used. If it's a plain object, it's considered as an options object also. I think that should cover all cases, but please review if I missed something.

(Wow, these web APIs has two many alternatives on how to call them)

[gc-victor]

I have added a couple of minor comments. I didn't test it, but it looks good. In an ideal world, these APIs should be tested using wpt. Deno and other runtimes use it.

(Wow, these web APIs has two many alternatives on how to call them)

It is hard to be compliant with the specifications.

Reference: https://github.com/web-platform-tests/wpt

[richarddavison]

Agree. That's why we added support for expect so LLRT tests can run vitest with node and if both passes we're good!

[gc-victor]

Agree. That's why we added support for expect so LLRT tests can run vitest with node and if both passes we're good!

I didn't check that part. I have to take a look into it.