Fetch options do not allow explicit `undefined`

[chlonel]

Bug Description

It's hard to say for sure that this is a bug, but it feels like unintended behavior, so I'm reporting it as a bug.

Passing redirect: undefined as an option of fetch will throw an error.
At the time of executing the following code, init.redirect may be assigned undefined, which will be considered as an invalid value.
This does not only happen with redirect, but also with other options such as method and cache.

// 22. If init["redirect"] exists, then set request’s redirect mode to it.
if ('redirect' in init) {
  request.redirect = init.redirect
  if (!requestRedirect.includes(request.redirect)) {
    throw new TypeError(
      `Failed to construct 'Request': The provided value '${request.redirect}' is not a valid enum value of type RequestRedirect.`
    )
  }
}

Reproducible By

fetch("https://github.com", { redirect: undefined });
// TypeError: Failed to construct 'Request': The provided value 'undefined' is not a valid enum value of type RequestRedirect.

Expected Behavior

If undefined is passed, it will simply be ignored and the default value will be assigned.
Although undefined should not be passed by right, it is useful if it works as well, since Google Chrome and node-fetch implementations do not throw errors.

Environment

Manjaro 21.2.1
Node.js v16.13.2

[ronag]

What does chrome do?

[ronag]

Good spot. I think we should replace x in init with init[x] != null everywhere. Feel free to open a PR (with test).

[chlonel]

I'm glad this is not the intended behavior.
I'll create PR later!

[chlonel]

There is a difference between Chrome and node-fetch.
Chrome does not allow redirect: null, while node-fetch ignores it as well as undefined.
I think we should only allow undefined if undici want to be compatible with Chrome, but which one is better?

[mcollina]

What does the spec says?

[chlonel]

Looking at the spec, I think it would be appropriate to throw an error for null.

https://fetch.spec.whatwg.org/#request-class
The procedure to set redirect is as follows:

22. If init["redirect"] exists, then set request's redirect mode to it.

https://infra.spec.whatwg.org/#map-exists
"exists" is described as follows:

An ordered map contains an entry with a given key if there exists an entry with that key. We can also denote this by saying that, for an ordered map map and key key, "map[key] exists".

If the key does not exist or is explicitly assigned undefined, they will return the same value.
There will be no problem with undefined being ignored.

null is also a value that indicates that the key does not exist, but the meaning is different from undefined which is returned when the key does not exist.
I am not familiar with the difference between undefined and null as a specification, but I think it is better to assume that the value null exists.

I read the spec before creating the question, but I am not certain of the difference between no key and explicit undefined as well.
So I was not sure if I should report it as a bug.

[mcollina]

I would treat a field with undefined value and no field in the same way.

[ronag]

Also I think Chrome treats null the same way as well...

[chlonel]

When I run the following code in the console, it throws an error.
Unfortunately, I don't think it is treated the same way.

fetch('https://github.com', { redirect: null })
// Uncaught (in promise) TypeError: Failed to execute 'fetch' on 'Window': Failed to read the 'redirect' property from 'RequestInit': The provided value 'null' is not a valid enum value of type RequestRedirect.
//     at <anonymous>:1:1

I understand that "chrome" is the Google Chrome browser, am I correct?

[ronag]

As an unofficial rule we try to match Chrome's behavior, including the actual error message.

[chlonel]

I see.
I'll fix it to only allow undefined.