navigation fails when external url is provided

NATUREFITNESS commented 1 year ago

Prerequisites

[X] I have written a descriptive issue title
[X] I have searched existing issues to ensure it has not already been reported

Version (i.e. v2.x.x)

1

Node.js version (i.e. 18.x, or N/A)

16

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10, or N/A)

13

Description

tested on ios safari and mac os safari (v 16), the relative url navigates without issue but a full external url does not navigate.

example

navigation.navigate('/login'); // works
navigation.navigate('https://gmail.com/login'); // link for external domain , doe not work

maybe I am missing something ?

Steps to Reproduce

safari v16 (ios or mac either is okay)
navigate to an external site with full url like navigation.navigate('https://github.com/virtualstate/navigation/issues/new?assignees=&labels=&template=bug.yml')

Expected Behavior

browser should redirect to new url

fabiancook commented 1 year ago

Ha interesting, I'm not even sure if an external navigation is part of the spec, surely if it accepts a url...

How are you integrating in? Are you using the thin polyfill or constructing a navigation class yourself?

fabiancook commented 1 year ago

Ah yeah I can see for sure that using navigation.navigate('https://gmail.com/login') with the native navigation in chrome does definitely navigate to the external url.

If you're using the polyfill it doesn't currently update the browsers url state.

For the browser this module is closer to an interim shim to get us through till all browsers have navigation implemented, the interactions between navigation & history aren't perfectly straightforward, so I had left the interaction out of this module.

This module would be more suited to servers or edge compute where navigation & routing is wanted, but a browser window doesn't exist.

If a more complex polyfill is wanted I can definitely look more into the interaction between the two.

As is, something like the below should be able to trigger the external navigation in a browser.

if (typeof location !== "undefined") {
  navigation.addEventListener("navigate", event => {
    const { destination: { url } } = event;
    const { origin } = new URL(url);
    if (origin !== location.origin) {
      event.intercept();
      location.href = url;
    }
  })
}

NATUREFITNESS commented 1 year ago

Thanks. I am currently already using your proposed solution to navigate to external urls. I am currently working on a native JS PWA app. Things are working fine on chrome but safari was causing problem in routing with navigation. so I tried your polyfill, that surely solves the problem, partially. as you mentioned, some parts of this poly-fill are still lacking the features that can be wished for. for example, the history part. but overall this is a very good work. thanks for posting it. at least it gets the job done in a non supported browser.

fabiancook commented 1 year ago

If you're using the polyfill in latest chrome, you might also be getting the current native version. I'll mention that chrome doesn't ship the current spec, so this module has both the old and new methods available, this mostly is the change from event.transitionWhile (old) to event.intercept (new) for the "navigate" event. You can for now completely use either or from this module.

.. so using the polyfill & native chrome navigation, you would want to be using navigation.addEventListener("navigate", event => event.transitionWhile(....)) for now.

Spot transitionWhile and canTransition on this NavigateEvent:

Before this module is "released" this function will be removed as part of a cleanup.

https://github.com/virtualstate/navigation/blob/f13775309d41dbc8e437d3aeb5709ed4006910a5/src/events/navigate-event.ts#L70-L75

This will be probably after at least one other browser has implemented the spec

NATUREFITNESS commented 1 year ago

thanks right now I am using it only for safari as chrome is already working without any polyfills. I was wondering if adding the ability to navigate to fully qualified external URLs will not break the implementation or will not go against the spec (as you can already do that in chrome ), can you allow / add that feature ? I could post a pull request too but I am not sure yet if this has anything to do with the specs of this api.

fabiancook commented 1 year ago

If you are looking to do a PR, for polyfill only additions you can add them to this spot here.

https://github.com/virtualstate/navigation/blob/f13775309d41dbc8e437d3aeb5709ed4006910a5/src/polyfill.ts#L20

In the core code there is no lock in with any browser API, so no actual usage of location or history.

It does reference its types though, the module is written for runtimes that have no attached UI, so location and history wouldn't be available to begin with.

If you were using any location or history code now and wanted to still use it server side, then you could do

import { NavigationSync } from "@virtualstate/navigation";
import navigation from "@virtualstate/navigation/polyfill";

const history = new NavigationSync(navigation),
  location = history; // Both apis are provided by this class.

This version is using navigation as a basis for history and location instead of the other way around... because its a lot "simpler"

NATUREFITNESS commented 1 year ago

Thanks for your tip. And yes, I ditched the history api already in favor of the navigation api. I will look into the navigation/src/polyfill.ts and see what can I push.

fabiancook commented 1 year ago

Hi, we have updated the polyfill to automatically integrate into the history API, but we have not yet got it integrated with external URLS, did you come up with a solution to have this work? Or did you need to go with something else?

If an anchor is used, in the polyfill, sameDocument will be false, and if intercept or preventDefault is called, the navigation will go ahead.

fabiancook commented 11 months ago

This has now been implemented. If you're utilising the polyfill, and a link that isn't the same document is found, a copy of the target element is made and navigated to, this allows navigation to intercept the event, but it utilises the normal browser behaviour for anchors and forms, for example taking into consideration target="_blank" or various form attributes.

virtualstate / navigation