WICG / web-app-launch

Web App Launch Handler
Other
75 stars 28 forks source link

launch_handler explainer feedback #56

Closed mgiuca closed 2 years ago

mgiuca commented 2 years ago

Feedback on the Launch Handler explainer.

alancutter commented 2 years ago

Thanks! Implemented changes here: https://github.com/WICG/sw-launch/pull/57

Feedback on the Launch Handler explainer.

  • Non-goals: "Multi-document-instance web apps: a web app that opens documents in their own instances but wishes to refocus an already open document instead of opening duplicate instances for it. This would instead by handled by the service worker launch event."

    • The implication from the front page is that this explainer subsumes SW launch event, so it shouldn't make that a non-goal.
    • Perhaps this (and the previous) point should be moved into a "future goals" section. Moved multi-document-instance example into the "possible extensions" section for SW launch event. By "and the previous point" do you mean the link capturing one?
  • LaunchParams

    • Should be a dictionary, not an interface.
    • targetURL should be required, and not nullable (?). Aha, here is where it came from. Fixed.
  • After "LaunchParams are buffered indefinitely until they are consumed.", should say "Crucially, if any events are buffered into a LaunchQueue before a call to setConsumer, they will be immediately passed into the consumer afterwards. This avoids a race condition between calling setConsumer and having launch events occur."

I don't really want to call them events since they don't follow how events are normally exposed to script. This paragraph already mentions avoiding the race. Added the first part with some rewording.

  • And: "Unlike event listeners, only a single consumer can be registered at a time. This ensures that each LaunchParams is passed to exactly one consumer." (Maybe we need to justify this further, or consider why we have this limitation.)

I'm not sure there's much reason other than it'd be weird language wise for something to be consumed multiple times. We could technically change it to be addConsumer/removeConsumer no problem I think.

  • After "Example usage for a single-window music player app" (following the example), say "Note that the app does not need to actually navigate to the URL; it can just inspect the URL and then adjust its page to a state as if it had navigated to the URL, which can often be done more efficiently, or without losing existing page state (for example, the existing song the user was listening to could be placed before the new song in a queue, so the user can easily go back to it)."

This example should really be navigating to the URL as a fallback! Hello nav black hole footgun... Updated and added comment.

  • In route_to: auto, add: "This reflects the status quo of user agent implementations prior to this proposal: mobile agents typically navigate an existing client, while desktop agents typically create a new one and avoid clobbering state."

Replaced with this text.

  • The navigate_new_clients/new_client_url "possible extension" isn't an extension, right, but an alternative proposal?

These are extensions. The current design only pertains to deduping launches with existing app instances. These new_client fields are for configuring what happens when we don't dedupe. On second thought with the folding of navigate_existing_client into route_to these would probably also fold into different new-client route_to values.

mgiuca commented 2 years ago

From discussion today:

alancutter commented 2 years ago

I'm leaning towards something like:

  "launch_handler": {
    "client_mode": "auto" | "new" | "navigate-existing" | "no-navigate-existing" | "via-service-worker"
  }
alancutter commented 2 years ago

These comments have now been addressed.