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

Calling listen() with an optional URL to be used on its internal run() call #25

Closed
paulocoghi opened this issue Mar 3, 2020 · 18 comments · Fixed by #26
Closed

Calling listen() with an optional URL to be used on its internal run() call #25

paulocoghi opened this issue Mar 3, 2020 · 18 comments · Fixed by #26

Comments

@paulocoghi
Copy link
Contributor

Hello, Luke!

I am implementing navaid as a long-lived router on an app that will be used both on web and smartphones (through Cordova).

Is there an option on navaid to avoid the automatic call of run() when calling listen() or a way to provide a specific URL to this first call, like / instead of the current URL?

This helps my app to run fine on smartphones as well, because otherwise the run() call will fail with the initial URL provided by Cordova, that is something like file:///home/user/app/directory/index.html.

I used Page.js to to this before, and on the smartphones I used its dispatch: false option to avoid this firs "run". But Navaid is so incredibly smaller that I prefer it over Page.js.

Thanks again!

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

Naturally the logic to identify if I am behind Cordova or not is on my side. (I've done this)

I would just like to know if it would be a problem to allow an optional argument to listen() to provide a desired URL instead of the current one on the browser.

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

If am thinking correctly, updating the line 44 and 74 with an optional u argument would allow this without breaking changes.

In the normal scenario, when calling listen() without arguments, the run() call will fallback to location.pathname as it already does.

	$.listen = function (u) {	// line 44

		// ...

		return $.run(u);	// line 74
	}

Only two additional bytes to navaid 😁 😄 😅

If you approve, I can make a PR.

@paulocoghi paulocoghi changed the title Possibility to listen() without automatically calling run() Calling listen() with an optional URL to be used on its internal run() Mar 3, 2020
@paulocoghi paulocoghi changed the title Calling listen() with an optional URL to be used on its internal run() Calling listen() with an optional URL to be used on its internal run() call Mar 3, 2020
@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

Hey hey :)

Would passing in / initially be enough? Does the rest of the (Cordova) app work beyond that first call?

Also, that'd be the wrong run – you'd have to pass u into here: https://github.com/lukeed/navaid/blob/master/src/index.js#L74

@paulocoghi
Copy link
Contributor Author

What a fast reply! I corrected the last comment to the line 74 :)

Would passing in / initially be enough? Does the rest of the (Cordova) app work beyond that first call?

Yes.

@paulocoghi
Copy link
Contributor Author

It is enough because the future (non-external) URL's to be called will continue to be managed by navaid normally.

And the external URL's will be opened outside the app, on the smartphone's default browser.

@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

Cool :) Go ahead and start a PR if you'd like. I can take care of the docs & tests if you prefer.

@paulocoghi
Copy link
Contributor Author

Ok!

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

As I've done in previous apps, I always set the app's internal links with an initial '/' so they will be not treated as relative URLs, no matter the route the user is navigating on.

Everything works perfectly on smartphones this way (when packaged by Cordova).

When publishing to smartphones, the URL of images are automatically prefixed with a variable to use the original path from the filesystem, on my personalized build phase, so we have zero issues with images as well.

@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

Yup, same. It's how most developers think of URLs so might as well write it that way too. If you know your base is some fixed /foobar/ path, then that's Navaid's base

Simple to reason about and implement

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

I can take care of the docs & tests if you prefer.

I would like to update the docs and tests as well!

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

Although all existing tests passed after the modification, testing the the listen function would require puppeteer or similar libraries, because it expected variables from the browser, like history.

I implemented tests with it on my browser-sync/livereload alternative project Liven.

But do you think it is relevant to the project that I implement the tests with puppeteer? It seems to me like using a sledgehammer to crack a nut. 😅

@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

No, I really don't want to be testing the browser.

You can test it by faking the method:

let expected;
let ctx = navaid();

ctx.run = function (uri) {
  t.is(uri, expected;, '~> run() received expected value');
}

ctx.listen(expected = undefined);
ctx.listen(expected = '/foo');

@paulocoghi
Copy link
Contributor Author

Ok! 👍

@paulocoghi
Copy link
Contributor Author

paulocoghi commented Mar 3, 2020

My attempt was similar, but wrap() is called inside listen() that tries to access the history variable and it fails because history is not defined.

But I wouldn't want to disturb you if this is taking your time. I'm thinking of a solution.

@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

I'm happy to do it, I just don't want both of us doing the same work haha

@paulocoghi
Copy link
Contributor Author

Ok, I will leave the tests for you. I really apologize 😅

@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

Lol don't sweat it bud, thank you

@lukeed lukeed closed this as completed in #26 Mar 3, 2020
lukeed added a commit that referenced this issue Mar 3, 2020
* Optional URL to the `listen()` call

Solves #25

* chore: add `listen()` tests

* chore: update docs

Co-authored-by: Luke Edwards <luke.edwards05@gmail.com>
@lukeed
Copy link
Owner

lukeed commented Mar 3, 2020

Now available as v1.1.0 🎉

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

Successfully merging a pull request may close this issue.

2 participants