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

what should Client.url return for a reserved Worker Client? #1034

Closed
wanderview opened this issue Dec 16, 2016 · 18 comments
Closed

what should Client.url return for a reserved Worker Client? #1034

wanderview opened this issue Dec 16, 2016 · 18 comments

Comments

@wanderview
Copy link
Member

So, the new reserved client support tries to let someone access a Client object for the thing that will be created by the current FetchEvent. So in theory you can do:

addEventListener('fetch', evt => {
  evt.waitUntil(clients.get(evt.reservedClientId).then(client => {
    console.log(`Reserved client ${evt.reservedClientId} has creation URL ${client.url}`);
  });
});

This works for a window Client because the creation URL is set on the environment based on the request in step 4 here:

https://html.spec.whatwg.org/#process-a-navigate-fetch

For workers, however, the Client creation URL is set based on the worker global scope URL in step 7 here:

https://html.spec.whatwg.org/#script-settings-for-workers

And the worker global scope URL is set based on the response URL after the script has been fetched. See step 10.3 here:

https://html.spec.whatwg.org/#worker-processing-model

Does this mean the reserved Client URL should be empty string for a Worker Client? Or should it be set to the request URL and then updated to the final response URL to account for redirects?

@wanderview
Copy link
Member Author

Barring other feedback I think I will make reserved Worker Client.url return the request URL.

@annevk
Copy link
Member

annevk commented Dec 17, 2016

I would slightly prefer empty string to discourage usage of the field at that point.

@wanderview
Copy link
Member Author

Yea, we could make Client.url return empty string whenever the Client.reserved is true. That would make things easier during redirects as well.

@jungkees
Copy link
Collaborator

jungkees commented Dec 19, 2016

I will make reserved Worker Client.url return the request URL.

I'd slightly prefer this indeed. I think the request URL is a good initial value for the environment's creation URL. And we can easily initialize the value by providing url to set up a worker environment settings object that I missed.

@jungkees jungkees added this to the Version 1 milestone Dec 19, 2016
@jungkees
Copy link
Collaborator

Looking at the steps, I think we don't even need to change set up a worker environment settings object at all. We can simply set the request's url to url in the perform the fetch algorithm before invoking fetch.

@annevk
Copy link
Member

annevk commented Dec 19, 2016

@jungkees did you miss my concern with that approach or do you not think it's problematic for some reason?

@jungkees
Copy link
Collaborator

jungkees commented Dec 19, 2016

I thought using client.url within fetch event handlers would be fine assuming we fix it to be initialized to the request's url in all cases.

I actually got confused while thinking over your question. For the clients that are not in reserved state, is client.url supposed to return the final response's url instead of the request's url? If so, I think it needs changes as well. And if we change that as such, client.url's value will dynamically change and its value for worker client redirect case would not be deterministic. Is this what you've concerned?

@annevk
Copy link
Member

annevk commented Dec 19, 2016

Yes, the URL of a "normal" client is the URL of its global object. Which would be the "final" URL.

And yeah, having it change around does not seem ideal. Better to start with a known weird value, such as the empty string.

@jungkees
Copy link
Collaborator

That makes sense. I'll fix it as suggested. When creating a Client object, if client’s execution ready flag is unset, client.url is set to the empty string, and the URL of its global object otherwise.

@jungkees
Copy link
Collaborator

Also, Create Window Client has bugs. It doesn't initialize the base interface part of the fields properly. Need to fix them.

@jungkees jungkees added the bug label Dec 19, 2016
@wanderview
Copy link
Member Author

I thought using client.url within fetch event handlers would be fine assuming we fix it to be initialized to the request's url in all cases.

Keep in mind you can get at these Client objects outside of the fetch event handler. The spec has Clients.match({includeReserved:true}) I believe.

@jungkees
Copy link
Collaborator

Yeah, clients.get(e.reservedClientId) already goes async in the fetch event handler as well indeed. So, the plan is to return a constant value (the empty string as proposed here although I think request's url also makes sense) until client’s execution ready flag is set and the URL of its global object after that moment. Those values are captured in Client creation time.

@wanderview
Copy link
Member Author

From an implementation point of view I prefer the empty string as proposed. I have some async work in gecko I want to perform to validate the URL against the origin. I can more easily do that at execution ready time vs immediately on creation. It also ensures we are using the final redirected URL instead of wasting work on request data that might get discarded.

@jungkees
Copy link
Collaborator

From an implementation point of view I prefer the empty string as proposed.

I understood your point. That works for me.

On the one hand though, I'm still not entirely clear here maybe because we haven't settled on what a Client (and an environment) should represent during redirects: #1031. It'll be nice if we can confirm the proposed behavior for this issue when #1031 is resolved.

@wanderview
Copy link
Member Author

I think we should also do something similar for other attributes on Client. For example:

  • Client.ancestorOrigins should return a zero length list
  • Client.frameType (if its still implemented) should return none.
  • Client.focused should return false

I'm not sure what Client.visibilityState should return for reserved clients. Should it be unloaded or preload?

Basically only Client.id and Client.type should return useful data on reserved Client objects.

Also, Client.focus() and Client.navigate() should probably reject with InvalidStateError. I think only Client.postMessage() should function in that it queues messages until execution ready.

@wanderview
Copy link
Member Author

I'm not sure what Client.visibilityState should return for reserved clients. Should it be unloaded or preload?

Well, apparently "unloaded" is not a thing any more according to the draft visibility spec:

https://w3c.github.io/page-visibility/#visibility-states-and-the-visibilitystate-enum

So I will use "hidden".

Side note, why does the SW draft spec link to the TR visibility spec instead of its draft?

@jakearchibald
Copy link
Contributor

F2F: reserved client should have an empty url, and we should come up with default values for each. Potential window navigations should be reserved window clients.

@jakearchibald
Copy link
Contributor

We aren't going to expose reserved clients, so this is no longer an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants