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

Case sensitive router #271

Closed
DamageLimiter opened this issue Jan 2, 2016 · 20 comments · Fixed by aurelia/route-recognizer#21 or #327
Closed

Case sensitive router #271

DamageLimiter opened this issue Jan 2, 2016 · 20 comments · Fixed by aurelia/route-recognizer#21 or #327

Comments

@DamageLimiter
Copy link

The routing is case sensitive and I am not quite sure if it's intended to be like this and url's should not be case sensitive.

I have this route:

{ route: "fertigung/waage/:id", moduleId: "fertigung/waage", name: "waage", title: "Waage", nav: false, settings: { hasSubMenu: false, isSubMenu: true, parent: 'Fertigung' }

and I have a link dependent on that route:
<a href="#/fertigung/Waage/${formular.Id}" class="btn btn-white btn-xs"> Waage</a>

As you can see I have written "Waage" with a capital "W" in the html link, which results to a redirect to the default route. Shouldn't the route itself be case insensitive?

@EisenbergEffect
Copy link
Contributor

@bryanrsmith I'm thinking that our pattern matching should be case-insensitive but that when we pass the parsed parameters on to the activate method, they should retain their original casing. What do you think?

@bryanrsmith
Copy link
Contributor

I agree.

@wichert
Copy link

wichert commented Feb 3, 2016

If this is changed I would like to have an option for the router to remain case sensitive.

@davidbfrogman
Copy link

I would also be interested in the router being case insensitive.

@tolemac
Copy link
Contributor

tolemac commented Apr 2, 2016

I have a strange behaivor with the router tests.

I have add after it('should navigate to named routes', this test:

it('should navigate to case sensitive routes', (done) => {
  const options = {};
  spyOn(history, 'navigate');

  router.configure(config => {
      config.map({ name: 'test', route: 'Test/Test', moduleId: './test' });
    })
    .then(() => {
      router.navigate('/Test/Test', options);
      expect(history.navigate).toHaveBeenCalledWith('#/Test/Test', options);
      history.navigate.calls.reset();

      router.navigate('/test/Test', options);
      expect(history.navigate).toHaveBeenCalledWith('#/test/test', options);
      history.navigate.calls.reset();


      done();
    });
});

And the new test passed ... Should this fail?

@davidbfrogman
Copy link

I'd love to see some movement on this feature.

@tolemac
Copy link
Contributor

tolemac commented Apr 4, 2016

I would like to help to implement it but have no experience with tests. Anybody can tell me how to test it in order to try implement it?

@EisenbergEffect
Copy link
Contributor

Take a look at some of the existing tests in the repo and see how they are put together. If you have questions, I'm sure @bryanrsmith can help you out.

@tolemac
Copy link
Contributor

tolemac commented Apr 4, 2016

Yes, I have looked to the rest of the test, and I have copied the should navigate to absolute paths test, I have wrote the avobe test and I think it works.

The problem is that it should fail but passed ... I don't understand :D. If I write the test with other route, for example:

it('should navigate to case sensitive routes', (done) => {
  const options = {};
  spyOn(history, 'navigate');

  router.configure(config => {
      config.map({ name: 'test', route: 'Test/Test', moduleId: './test' });
    })
    .then(() => {
      router.navigate('/Test/Test222222', options);
      expect(history.navigate).toHaveBeenCalledWith('#/Test/Test', options);
      history.navigate.calls.reset();

      router.navigate('/test/Test22222222', options);
      expect(history.navigate).toHaveBeenCalledWith('#/test/test', options);
      history.navigate.calls.reset();


      done();
    });
});

Both fails, the problem is when I try to test the case sentive route navigate its works ... like if the router is case insensitive ... I don't get it to fail with case sensitive routes ...

@tolemac
Copy link
Contributor

tolemac commented Apr 4, 2016

I'm thinking ... should be this issue related to "route-recognizer"??
I think yes.

@tolemac
Copy link
Contributor

tolemac commented Apr 5, 2016

I have created a pull request on "route-recognizer" to be case insensitvie: aurelia/route-recognizer#20

If this pull request is accepted will be easy to enable it by each route.

@EisenbergEffect
Copy link
Contributor

@bryanrsmith or @jedd-ahyoung Can you review the PR?

@bryanrsmith
Copy link
Contributor

@EisenbergEffect Left a few comments for minor issues.

I'm not sure about the API, though. It looks like we're adding an API for specifying whether or not the router is case sensitive, and then letting you specify an override on each individual route. IMO we should just make the router case insensitive by default, and let routes specify otherwise. I think that would be a less surprising default, and putting the only API on the route config means I don't have to look elsewhere to figure out how a particular route will behave.

@tolemac
Copy link
Contributor

tolemac commented Apr 13, 2016

@bryanrsmith I'm agree with your comments on the PR.

I'm agree about API notes too, if @EisenbergEffect is agree too I can make the changes.

@EisenbergEffect are you agree?

@EisenbergEffect
Copy link
Contributor

@tolemac Yes, I agree. Let's proceed 😄

Also, I noticed that you haven't yet signed our CLA. We would appreciate it if you would consider doing that for us. Before signing, please take a few minutes to read our contributing guidelines. These guidelines will help get you up to speed on how our community likes to operate and how the pull request process works. Once you have read that, assuming you are in agreement, simply sign our online CLA.

Please CC myself and @bryanrsmith when you have signed the CLA and made the change that bryan recommends.

@tolemac
Copy link
Contributor

tolemac commented Apr 16, 2016

@EisenbergEffect and @bryanrsmith my apologies, I didn't read the contributing guidelines. Well, I have read it now, and I have cancel the pull request and I have done other following the guidelines.
I have signed the CLA too.

Here is the new pull request: aurelia/route-recognizer#21

@tolemac
Copy link
Contributor

tolemac commented Apr 16, 2016

Well, the changes on route-recognizer aren't enougth to close this issue.
We need to make changes on the router too. I think to add "caseSensitive" property to the RouteConfig interface. Then, on Router.addRouter method pass this property to route recognizer:

let state = this._recognizer.add({path: path, handler: config, caseSensitive: config.caseSensitive});

I'm trying to write a spec to test the router to navigate to case sensitive routes.

@tolemac
Copy link
Contributor

tolemac commented Apr 16, 2016

@bryanrsmith, @EisenbergEffect I just finished the changes on the router. I have replace the route-recognizer module by the new and it works.

How to do it? have I to wait to you accept the route-recognizer and then make the pull request to the router??

@EisenbergEffect
Copy link
Contributor

@bryanrsmith Can you review?
@tolemac Go ahead and add a PR the second set of changes to the router.

@tolemac
Copy link
Contributor

tolemac commented Apr 16, 2016

Done.

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