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

Incorrect root detection #138

Closed
VeXell opened this issue Jun 10, 2017 · 11 comments
Closed

Incorrect root detection #138

VeXell opened this issue Jun 10, 2017 · 11 comments

Comments

@VeXell
Copy link

VeXell commented Jun 10, 2017

I use last version 5.1 and webpack dev server with these settings
devServer: { contentBase: path.join(__dirname, "build"), compress: true, host: "0.0.0.0", disableHostCheck: true, port: 9000, historyApiFallback: true }

I init Navigo with default parameters (root= null). In previous version 4.8 after reload page Navigo detects root path well and show me right page. But now, after page reload, it shows me my home page again.

For example when i start page with URL "http://localhost:9000/schedule/3304" it thinks that root path is "/schedule/3304". But it is not correct.

I think this function has error:

_getRoot: function () { if (this.root !== null) return this.root; this.root = root(this._cLoc().split('?')[0], this._routes); return this.root; },

UPD: To fix this problem i should init Navigo with first parameter location.origin. I think u can add it to code to fix it too.

@krasimir
Copy link
Owner

Finding the root URL was always tricky. The 5.x versions actually have a better route matching but maybe that part with guessing the root is not working the same way. I'm considering making that root parameter mandatory and we always have to provide it. In most of the cases the developer knows where the app is located.

Can you please post here your route definitions so I can reproduce your problem.

P.S.
I don't want to rely on location.origin because as far as I can see it is not supported everywhere.

@VeXell
Copy link
Author

VeXell commented Jun 13, 2017

It can be used in most modern browsers, i think.
My routes are very simple:

router.on('/session/:sessionId', (parameters) => this.executeController(SessionIndexController, parameters));
router.on('/schedule/:filmId', (parameters) => this.executeController(ScheduleIndexController, parameters));
router.on('/film/:filmId', (parameters) => this.executeController(FilmIndexController, parameters));
router.on('/select-city/:cityId', (parameters) => this.executeController(CitySelectController, parameters));
router.on('/select-city', (parameters) => this.executeController(CityIndexController, parameters));
router.on('/list/:when', (parameters) => this.executeController(HomeController, parameters));
router.on('/', (parameters) => this.executeController(HomeController, parameters));

@jeremenichelli
Copy link

I think this issue is super serious because it basically kills the route initialization itself whenever is not home, location.origin is supported since Safari 7 and IE11.

If you want to support older browsers, location origin can be easily covered by combining location.protocol, location.host and location.port.

@krasimir
Copy link
Owner

krasimir commented Oct 7, 2017

So I started refactoring the router to use location.origin. However, that breaks a lot more stuff and I realized that it is against one of my core ideas. Navigo should work if you drop it on a page even tho that page is deeply nested. For example http://site.com/myapps/name/run.html. If we say that we are using location.origin as a root then the router will assume that the path for matching is myapps/name/run.html. So, I'll leave this issue open and will consider it when I start working on a new version.

P.S.
Navigo started as a small script for managing routing but became full with patches because it tries to cover lots of cases. I'm considering rewriting it in a simple form.

@jeremenichelli
Copy link

jeremenichelli commented Oct 7, 2017

Hi @krasimir, I agree with the drop-in view you have about a router, no matter how deeply nested the route is. I think that is a big differenciator from other solutions, however I feel that nested path should be specified by the developer so the router works as expected when the application doesn't start from home.

For example, I have a web app with two routes, / for home and movie/:id for some speific section, if the user instead of accessing to home directly it goes to www.some.site.com/path/to/movie/286 the router kicks off at that path as home and to access a movie the url would be www.some.site.com/path/to/movie/286/movie/286.

I think that taking location.origin as base and a path specified by the developer will solve this compromise. Let me know if this insight helps, I would love to help you out on the re-write.

Cheers!

@krasimir
Copy link
Owner

krasimir commented Oct 7, 2017

I'm in a constant battle with myself about how to design the router. What you just described is the second variant. The first one is how the router works now. Last couple of months I'm thinking about a rewrite of the whole thing and I may change it indeed as you are suggesting. It's just with the current code base will be nightmare. There are so many things that will break if we start using location.origin.

@jeremenichelli
Copy link

Yeah, I agree it will mess up with a lot of stuff. Probably a re-write would be best, and check current features as you tackle them in the new version. Either way, breaking changes and semver exist for something 😄

@jpenna
Copy link

jpenna commented Jul 27, 2018

This suggestion of location.origin should be written at the documentation, and the backward compatible choice of location.protocol, location.host and location.port too.

The documentation says we should set the entry point on new Navigo() but doesn't suggest how. I was trying location.host (which didn't work) and hadn't think about location.origin until I saw this issue.

And thanks for the router :)

@krasimir
Copy link
Owner

@jpenna What I meant by entry point was the root param of the constructor. In general, the next version will rely completely on that root param and it will be made mandatory, because trying to guess what it is is buggy.

@jpenna
Copy link

jpenna commented Jul 29, 2018

@krasimir What I am saying is that it should be written here:

The constructor of the library accepts three argument - root, useHash and hash. The first one is the main URL of your application. If you call the constructor without parameters then Navigo figures out the root URL based on your routes. However, this proves to lead to bugs so I strongly recommend to set a root value: a known root value or if you want to set it dinamically you can use location.origin or build the root string with location.protocol, location.host and location.port.

I am using location.origin in my project, why do you say it is buggy to guess it? Shouldn't I use location.origin?

@krasimir
Copy link
Owner

krasimir commented Dec 23, 2020

Finding the root URL was always tricky. The 5.x versions actually have a better route matching but maybe that part with guessing the root is not working the same way. I'm considering making that root parameter mandatory and we always have to provide it. In most of the cases the developer knows where the app is located.

Can you please post here your route definitions so I can reproduce your problem.

Ok so, 3+ years later I'm almost done with the re-write :) . The new version 8.0.0 (you can get it atm via yarn add navigo@beta) has a mandatory root param.

const router = new Navigo("/schedule");

router
  .on("/:id", (match) => {
    console.log("The id is " + match.data.id);
  })
  .resolve();

meaning that:

  • if you hit the server at "/" nothing happens
  • if you hit the server at "/schedule" nothing happens
  • if you hit the server at "/schedule/xxx" your handler is invoked
  • if you hit the server at "/schedule/xxx/boo" nothing happens

The example is here https://github.com/krasimir/navigo/tree/big-rewrite/examples/138

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