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 fetch options+timeout to FetchClient. #5101

Merged
merged 10 commits into from Sep 19, 2021

Conversation

rpiaggio
Copy link
Contributor

Add timeout capability to FetchClient and allow passing all the options present in RequestInit.

Copy link
Member

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, glad this module is getting attention!

But hang on—aren't many of these "options" simply headers that should be provided in the request? Apologies if I am missing something.

Additionally I'm very 👎 on using JS APIs here (e..g., js.UndefOr). This is not meant to be a facade, but an idiomatic http4s client. So, should use standard Scala classes e.g. Option and maybe even implement a FetchClientBuilder pattern used in the other http4s modules

@armanbilge
Copy link
Member

Also, since these options are per-request instead of being used for the FetchClient constructor they should be provided as attributes on the request itself.

@rpiaggio
Copy link
Contributor Author

rpiaggio commented Aug 23, 2021

Thanks for the PR, glad this module is getting attention!

Thank you for implementing this.

aren't many of these "options" simply headers that should be provided in the request?

Mostly they control how the Fetch client will interact with the browser. See https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#parameters

So, should use standard Scala classes e.g. Option and maybe even implement a FetchClientBuilder pattern used in the other http4s modules

I now changed the parameters to Option and implemented the Builder pattern. Note that this is a breaking change for people that were using FetchClient directly.

Also, since these options are per-request instead of being used for the FetchClient constructor they should be provided as attributes on the request itself.

I'm not so sure about this. In JS there's really no client instance, so it's up to us whether to interpret Fetch.fetch parameters as belonging to the client or the request. It seems to me it's more convenient for them to be client properties, insofar as requestTimeout is also a client property and not a request one, especially now that we have FetchClientBuilder. For example, we now have a

private val client = FetchClientBuilder[F]
    .withCredentials(RequestCredentials.include)
    .resource

that we use for different requests.

@armanbilge
Copy link
Member

Thanks for iterating on my feedback!

In JS there's really no client instance, so it's up to us whether to interpret Fetch.fetch parameters as belonging to the client or the request.

Ok, this is a fair point. How about this: what if we define these as per-request attributes, and then in the fetch client builder they can also provide some default attributes? Also:

Comment on lines 78 to 79
Resource
.eval(F.delay(new AbortController()))
Copy link
Member

Choose a reason for hiding this comment

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

This AbortController is an important addition, thanks! Instead of evaling it into the resource, maybe it should become part of the resource itself? I.e., in Resource.makeCase so that if the Resource is cancelled or errored we abort the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@armanbilge
Copy link
Member

Reading about the integrity parameter:

Subresource Integrity (SRI) is a security feature that enables browsers to verify that resources they fetch (for example, from a CDN) are delivered without unexpected manipulation. It works by allowing you to provide a cryptographic hash that a fetched resource must match.

This is definitely a per-request option.

@rpiaggio
Copy link
Contributor Author

Ok on all comments. How does this look?

  • requestTimeout: client (I think it would be trivial to support at request level too, but that may be inconsistent with other clients).
  • cache: client & request.
  • credentials: client & request.
  • integrity: request.
  • keepalive: client & request.
  • mode: client & request.
  • redirect: client & request.
  • referrer: client & request.
  • referrerPolicy: client & request.

Although there are indeed referrer and referrerPolicy headers, they don't behave the same way when passed as parameters as when passed as headers. As far as I can tell:

  • referrer allows to set a specific URL (which would be the same as setting the header), but it also allows other behaviors. Notably, passing "client" derives the Referer header from the current client URL.
  • referrerPolicy seems to be a header usually used in responses to indicate when the browser will send the Referer header in future requests. I can't think of a use as a request header. In this case, the parameter is used to indicate when the Referer header will be used in the Fetch request, not to set the header.

https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer

@isomarcte
Copy link
Member

@rpiaggio @armanbilge there are some request headers which are forbidden to be set in the init.headers section, Referer for example is one.

https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name

@armanbilge
Copy link
Member

there are some request headers which are forbidden to be set in the init.headers section, Referer for example is one.

Thanks, that's a good point. I think the idiomatic thing to do here is let users set that as a header, and add logic in the client to extract it out.

@armanbilge
Copy link
Member

@rpiaggio 👍 to your list. The only other one I think should be request-only is keepalive, this seems like something that should be used conservatively.

The keepalive option indicates that the request may “outlive” the webpage that initiated it.

Also setting it for the entire client may cause problems:

We can’t send megabytes: the body limit for keepalive requests is 64KB ... This limit applies to all keepalive requests together. In other words, we can perform multiple keepalive requests in parallel, but the sum of their body lengths should not exceed 64KB.

Via https://javascript.info/fetch-api#keepalive.

Regarding Referer, thanks for explaining, lots of subtleties to those I missed. 👍 and maybe we should look for the header and extract its value into init, if the user specifies it that way?

@rpiaggio
Copy link
Contributor Author

maybe we should look for the header and extract its value into init, if the user specifies it that way?

I'm on the fence about this. What would happen if it's both on the headers and in options?

@armanbilge
Copy link
Member

What would happen if it's both on the headers and in options?

Good question. By options, I assume you mean the per-request attributes? Request-level config should always have priority. I think the priority should be (request attribute) > (request header) > (client config). The thinking is, a user should be able to set the Referer header idiomatically (as it would for any client) without needing to know that it's actually using the fetch client.

On the flipside, suppose we only set Referer either by client parameter or per-request attribute. What happens when there is a Referer header provided in the request? Silently ignore? Raise an error because it's forbidden?

@rpiaggio
Copy link
Contributor Author

By options, I assume you mean the per-request attributes?

Yes.

Request-level config should always have priority.

Agreed. That's how I implemented it.

a user should be able to set the Referer header idiomatically (as it would for any client) without needing to know that it's actually using the fetch client.

Agreed that this is desirable.

On the flipside, suppose we only set Referer either by client parameter or per-request attribute. What happens when there is a Referer header provided in the request? Silently ignore? Raise an error because it's forbidden?

In this case Fetch will do whatever it does when it receives a Referer header. Probably throw an exception.

I think the priority should be (request attribute) > (request header) > (client config).

Hmm.. I implemented an override logic between the client and the request parameters. Prioritizing as you suggest would interfere with that logic, with the header getting in the way of the override. Would it be OK if it was (request header) > (request attribute) > (client config); or even (request attribute) > (client config) > (request header)? Alternatively, we could raise an error if it's present both in headers and request options.

@armanbilge
Copy link
Member

@rpiaggio can't we just do option-chaining like this? (pseudocode)

request.attributes.lookup(RefererKey)
  .orElse(request.headers.get(`Referer`))
  .orElse(client.attributes.lookup(RefererKey))

@rpiaggio
Copy link
Contributor Author

@rpiaggio can't we just do option-chaining like this? (pseudocode)

Yes, we definitely can add exceptional logic to handle this particular option. IMHO, header should have the least priority. We are setting other client and/or request option after all. But I'll go ahead and implement it as middle priority.

@armanbilge
Copy link
Member

IMHO, header should have the least priority.

Maybe, but it violates the philosophy that request-level option should have priority. The situation I'm thinking about is when a FetchClient is constructed but then provided to code that doesn't know anything about fetch, but uses this option. But these are all hypothetical and rare situations of course :) hopefully someone else can weigh in too with their opinion, @isomarcte maybe?

Yes, we definitely can add exceptional logic to handle this particular option.

This option truly is an exceptional one, precisely because we also need to check the headers :)

@rpiaggio
Copy link
Contributor Author

It dawned on me that we should also have a type for the referrer option, instead of just having a String as exposed in the facade. So I also added FetchReferrer.

@isomarcte isomarcte self-requested a review August 23, 2021 22:28
@rpiaggio
Copy link
Contributor Author

An unrelated (TomcatServerSuite) seems to have failed in one of the setups. Is this sporadic? Can someone with enough privileges re-run its jobs?

@armanbilge
Copy link
Member

armanbilge commented Aug 23, 2021

An unrelated (TomcatServerSuite) seems to have failed in one of the setups. Is this sporadic? Can someone with enough privileges re-run its jobs?

Don't worry about it, just general flakiness :) the chrome tests are the important ones. Also firefox if you can run locally? Otherwise I'll do it at some point before we merge.

@rpiaggio
Copy link
Contributor Author

I did a publishLocal and tested application where we use the snippet I copied above, which uses .resource and .withCredentials, in addition to .create and .withCache in other places. It seems to work fine.

@rpiaggio
Copy link
Contributor Author

BTW, I'm having trouble running locally the tests in FetchClientSuite. Is there something I should set up?

[error] Failed: Total 12, Failed 12, Errors 0, Passed 0
[error] Failed tests:.FetchClientSuite 0s
[error]         org.http4s.dom.FetchClientSuite
[error] (Test / testOnly) sbt.TestsFailedException: Tests unsuccessful

@armanbilge
Copy link
Member

Oh, oops. Yes. Run ./scripts/scaffold_server.js and ./scripts/static_server.py to start the servers. You also need to do in sbt: set useJSEnv := JSEnv.Firefox or set useJSEnv := JSEnv.Chrome. And install geckodriver for firefox or chromedriver for chrome 😅

Is this ready for another look?

@rpiaggio
Copy link
Contributor Author

Thank you.

Yes, it's ready for review.

Comment on lines 36 to 47
F.fromPromise {
F.delay {
val init = new RequestInit {}
F.delay(new AbortController()).flatMap { abortController =>
val requestOptions = req.attributes.lookup(FetchOptions.Key)
val mergedOptions = requestOptions.fold(options)(options.overrideWith)
Copy link
Member

Choose a reason for hiding this comment

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

Regarding the AbortController: I was imagining something more like this.

Resource.makeCase(F.delay(new AbortController)) {
  case (controller, Resource.ExitCase.Succeeded) => F.unit
  case (controller, Resource.ExitCase.Error(ex)) => F.delay(controller.abort())
  case (controller, Resource.ExitCase.Cancelled) => F.delay(controller.abort())
}.flatMap { abortController =>
  Resource.makeCase(/* fetch code in here */) { case (response, exitCase) =>
    closeReadableStream(response.body, exitCase)
  }
}.timeoutTo(d, F.raiseError(/* etc */))

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, on second thought I see that this is flawed, sorry. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think there's still some trickyness here. So in Resource.makeCase(acquire)(release) acquire is atomic i.e. uncancelable. So even if the request is canceled externally, it will still block until the fetch promise completes/times out. I think we want to use makeCaseFull and wrap the entirety of acquire inside of the poll, because it is cancelable thanks to your addition of the AbortController.

https://typelevel.org/cats-effect/docs/typeclasses/monadcancel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very good catch. I admit I forgot about atomic acquires. I'll look into it.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

I think this looks really great!! Huge leap forward from my initial implementation; I'm particularly happy you added the AbortController. Thanks for all of your effort 😀

I'll run the tests against Firefox later today or tomorrow. Unfortunately a lot of the good stuff in here isn't being tested in the suite, one day though hopefully :)

Comment on lines 36 to 39
object FetchClient {

def apply[F[_]](implicit F: Async[F]): Client[F] = Client[F] { req: Request[F] =>
private[dom] def makeClient[F[_]](
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we make FetchClient to private[dom]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the pattern from BlazeClient here, but yes, we can make it private. It just encapsulates the builder method.


/** Creates a [[Client]].
*/
def create: Client[F] = FetchClient.makeClient(
Copy link
Member

Choose a reason for hiding this comment

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

Name it build? Like EmberClientBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the pattern from JavaNetClientBuilder and OkHttpBuilder, which have a resource method for building a Resource (the method is inherited from BackendBuilder), and create to return a Client.

I see that EmberClientBuilder does not inherit from BackendBuilder and build returns a Resource. Maybe we could make it a BackendBuilder subclass and rename build -> resource there? Let me know if you want me to add that to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there are some discrepancies between the various implementations of both the client and server builders. There is some work to try to clean that up, but for now it is still kind of a free for all.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, I see. I tend to look to ember for what to do since my understanding is that it's the blessed server/client implementation. Sounds like this hopefully will be cleaned up and made more uniform as 1.0 finalizes.

Comment on lines 23 to 33
sealed trait FetchReferrer extends Renderable
object FetchReferrer {
Copy link
Member

Choose a reason for hiding this comment

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

This was definitely the right thing to do :)

Suggested change
sealed trait FetchReferrer extends Renderable
object FetchReferrer {
sealed abstract class FetchReferrer extends Renderable
object FetchReferrer {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem with making this change, but I'm curious why an abstract class is preferable to a trait in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Eh, just my own superstitious voodoo 😆 the binary encoding and runtime behavior of traits is/was slightly more complex than an abstract class. So I prefer to be conservative for sealed hierarchies since we have full control and only reach for trait if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

@armanbilge I'm of the same opinion as you, though I think on Scala >= 2.12.x if it is sealed then the differences are very minor between an abstract class and a trait. The story was different on 2.11.x when Scala wasn't using the JVM based default methods in interfaces.

init.method = req.method.name.asInstanceOf[HttpMethod]
// Referer header is converted to a Fetch parameter below, since it's a forbidden header.
// See https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name
init.headers = new Headers(
Copy link
Member

Choose a reason for hiding this comment

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

@rpiaggio it appears, at least in firefox, the behavior of the fetch client for forbidden headers is just to ignore them. If that is the case in general, I don't think you need to filter here.

Copy link
Member

Choose a reason for hiding this comment

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

This would be great for performance, but also makes me nervous about the one browser that does something annoying here!

Copy link
Member

Choose a reason for hiding this comment

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

The spec seems to indicate that forbidden headers will be silently ignored if appended. Ideally every browser implements the spec 🙃

Otherwise, if headers’s guard is "request" and name is a forbidden header name, return.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it seems there is no guard when we do new Headers

When a new Headers object is created using the Headers() constructor, its guard is set to none (the default).

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Basic_concepts#guard

So, seems the headers will be appended, and it's up to fetch itself to not balk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chrome also ignores. Removing the filter.


/** Creates a [[Client]].
*/
def create: Client[F] = FetchClient.makeClient(
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there are some discrepancies between the various implementations of both the client and server builders. There is some work to try to clean that up, but for now it is still kind of a free for all.

referrerPolicy = referrerPolicy
))

def resource: Resource[F, Client[F]] =
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an override here?

val redirect: Option[RequestRedirect],
val referrer: Option[FetchReferrer],
val referrerPolicy: Option[ReferrerPolicy]
)(implicit protected val F: Async[F])
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an override here since IIRC we get this from the BackendBuilder?

referrer: Option[FetchReferrer] = None,
referrerPolicy: Option[ReferrerPolicy] = None
) {
private[dom] def overrideWith(other: FetchOptions): FetchOptions =
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think a public merge function would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about naming it merge initially but it doesn't make it clear which side "wins" in case something is defined in both of them. We can still make it public.

Copy link
Member

Choose a reason for hiding this comment

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

I think as long as it's well-defined/documented it's fine. See also the merge functionality on vault, I forget what it's called.

Copy link
Member

Choose a reason for hiding this comment

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

@rpiaggio I'm fine either way.


sealed trait FetchReferrer extends Renderable
object FetchReferrer {
final case object NoReferrer extends FetchReferrer {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: final is redundant here

final case object Client extends FetchReferrer {
override def render(writer: Writer): writer.type = writer << "client"
}
final case class URL(uri: Uri) extends FetchReferrer {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be just a path, since the docs say it must be a same origin URL?

referrer
A USVString specifying the referrer of the request. This can be a same-origin URL, about:client, or an empty string.

@armanbilge do we have access to the host when we create the Fetch client? I'm still coming up to speed on Scalajs, but if we have the host then we can use that to set the referrer to <HOST>/<PATH>. If we take in a full Uri it will be ignored if it is not the same origin, which is probably not ideal.

Copy link
Member

Choose a reason for hiding this comment

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

@isomarcte yes, in the browser we do have access to window.location.host for the window and also self.location.host for web workers. For FetchClient to work in both of these browser environments we'll have to do this carefully 🤔

But 👍 on using a path here instead, it narrows statically to only valid inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely agree on this.

It may be tricky to support it correctly on all platforms though. As @armanbilge mentioned, we should proceed differently whether we are in window or web worker. Also, do we want to support node-fetch? (I have no idea what node-fetch does with the referrer option).

Copy link
Member

Choose a reason for hiding this comment

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

Nope, no node-fetch. We already have ember client running on node for those who need that. The goal here is purely to support browsers.

override def render(writer: Writer): writer.type = writer << "no-referrer"
}
final case object Client extends FetchReferrer {
override def render(writer: Writer): writer.type = writer << "client"
Copy link
Member

@isomarcte isomarcte Aug 24, 2021

Choose a reason for hiding this comment

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

This should be about:client.

In firefox, setting this to client causes the Referer to be <BASE_URL>/client, but about:client sets it to <BASE_URL>/fetch which I believe is the intended semantic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gosh, I was following this spec which states:

A request has an associated referrer, which is "no-referrer", "client", or a URL. Unless stated otherwise it is "client".

I hope at least all browsers follow the same spec.

Copy link
Member

Choose a reason for hiding this comment

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

Ouch. Navigating these specs and browsers is frustrating.

sealed trait FetchReferrer extends Renderable
object FetchReferrer {
final case object NoReferrer extends FetchReferrer {
override def render(writer: Writer): writer.type = writer << "no-referrer"
Copy link
Member

@isomarcte isomarcte Aug 24, 2021

Choose a reason for hiding this comment

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

This should be "", unless I'm mistaken.

In firefox, setting this to no-referrer causes the Referer to be <BASE_URL>/no-referrer while "" causes the request to not have a referer header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested in Chrome and luckily it behaves the same as in Firefox, so I'm fixing that. Also changing the custom option to wrap a Path. Also luckily, a path can be passed and the browsers take care of building the URL. In node-fetch the referrer option is ignored by design.

* @param referrer referrer of the request, this can be a same-origin URL, about:client, or an empty string.
* @param referrerPolicy referrer policy to use for the request
*/
final case class FetchOptions(
Copy link
Member

Choose a reason for hiding this comment

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

Would you be opposed to making this similar to RequestPrelude, e.g. not having the case class be public? case class encodings which aren't just arity 1 wrappers interact poorly with binary compatibility.

@isomarcte
Copy link
Member

@rpiaggio a lot of my feedback here is subjective, but I do think we need to address the FetchReferrer behavior discrepancy I'm seeing, e.g. client vs. about:client, "" vs no-referrer.

@rpiaggio
Copy link
Contributor Author

@rpiaggio a lot of my feedback here is subjective, but I do think we need to address the FetchReferrer behavior discrepancy I'm seeing, e.g. client vs. about:client, "" vs no-referrer.

Thanks for the review, I'll check the FetchReferrer in Chrome and address the suggestions.

@rpiaggio
Copy link
Contributor Author

@isomarcte @armanbilge I think all issues have been addressed. Can you take another look?

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

I think it's LGTM. I ran the dom suites on Firefox ✅

* @param referrer referrer of the request
* @param referrerPolicy referrer policy to use for the request
*/
sealed abstract class FetchClientBuilder[F[_]] private (
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could just use a final class and then avoid the {} in your anonymous instances.

Copy link
Contributor Author

@rpiaggio rpiaggio Aug 29, 2021

Choose a reason for hiding this comment

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

Sure. Just following the pattern of other parts of the codebase. See for example BlazeClientBuilder.

def referrerPolicy: Option[ReferrerPolicy]

def withCacheOption(cache: Option[RequestCache]): FetchOptions
def withCache(cache: RequestCache): FetchOptions =
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I've a very tiny preference for making these final here. It makes it easier to scan the class at a glance and see what is abstract vs. derived.

import org.http4s.util.Writer
import org.http4s.util.Renderable

sealed abstract class FetchReferrer extends Renderable
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a bit of Scaladoc here explaining the situation and/or a link to the MDN docs? As we've seen in discussing this PR the semantics of how this works isn't obvious at first glance.

.flatMap(_.referrer)
.orElse(req.headers.get[Referer].map(_.uri))
.orElse(options.referrer)
.foreach(referrer => init.referrer = referrer.renderString)
Copy link
Member

Choose a reason for hiding this comment

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

In the case this is the Path based encoding, we need to concat it with the window.location.host unless I'm mistaken.

I'm not exactly sure how that is done with here, e.g. is it an effect or statically scoped in the runtime. Sorry, still coming up to speed on Scalajs stuff.

@armanbilge can you comment here on how this can/should be done?

Copy link
Member

Choose a reason for hiding this comment

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

@rpiaggio said this works ok in #5101 (comment) without concatenating with host. Is this described in the spec by any chance?

I'm not exactly sure how that is done with here, e.g. is it an effect or statically scoped in the runtime.

Err, I guess it's statically scoped 🤔 JS is weird and dynamic. The bigger problem is that window.location.host isn't actually available in web worker environments, which instead have self.location.host I think. So if we can avoid this messiness altogether that would be preferable.

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find any mention of this behavior in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this described in the spec by any chance?

Yes. Kind of. The spec mentions that the parameter can have a URL from the current origin. Unless I'm missing something, a Path is a valid relative URL (and thus a URL). And it's interpreted by the browser relative to the current origin, so I'd say we are safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if we can avoid this messiness altogether that would be preferable.

I couldn't agree more.

Copy link
Member

Choose a reason for hiding this comment

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

a Path is a valid relative URL (and thus a URL).

Ah, it is? Excellent then!

Copy link
Member

Choose a reason for hiding this comment

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

👍

@armanbilge
Copy link
Member

@rpiaggio would you mind merging in or rebasing on main? I brought back CI for Firefox in #5123, would be good to run that here.

@rpiaggio
Copy link
Contributor Author

@rpiaggio would you mind merging in or rebasing on main? I brought back CI for Firefox in #5123, would be good to run that here.

👍 done