search

TehShrike / abstract-state-router

The best way to structure a single-page webapp.
http://tehshrike.github.io/state-router-example
296 stars 26 forks source link

Fixed an issue with `stateIsActive` that would crash the router #112

Closed taylorzane closed 7 years ago

taylorzane commented 7 years ago

If opts is null, this check fails as typeof null === 'object'.

This lets users use null or undefined as params. This probably needs to be a check to ensure that opts is a POJO, as some people could potentially pass a primitive. There isn't a lot of error throwing for type checking in the project, so something like throwing an error that you must use a POJO may feel out of place.

TehShrike commented 7 years ago

Could you add a new failing test for passing in null? In this pull request, or a separate one, either is fine. Probably before/after here. Feel free to copy one of those tests as a template.

I'm cool with asserting "if opts is null, it should be ignored".

taylorzane commented 7 years ago

Sure thing, I'll do this first thing in the morning!

taylorzane commented 7 years ago

Finally added the test. Let me know if you'd like a more verbose test.

TehShrike commented 7 years ago

huh, the diff got weird - did a rebase happen at some point?

Unrelated, this will probably run into some difficulties with #115

TehShrike commented 7 years ago

This is the current change in that modernization branch https://github.com/TehShrike/abstract-state-router/pull/115/files#diff-168726dbe96b3ce427e7fedce31bb0bcR366

I see that there are some issues with the current stateIsActive implementation - it only compares keys on the input object, but it should really be iterating over every key that applies to the state name that was passed in.

That's a separate bug, though - I'll update the implementation in that branch to be better backwards compatible

taylorzane commented 7 years ago

Ah shoot. I have rebase on pull enabled by default globally. Let me see if I can resolve that in my fork...

taylorzane commented 7 years ago

Alright, that's fixed. Force reset my branch back to my original commit in this PR, and re-added the test.

TehShrike commented 7 years ago

I ended up implementing a similar fix in the modernization branch (PR) - I'd refactored stateIsActive there, including adding a default argument. I think my last few commits should fix this issue, and also be reasonably backwards-compatible.

I added "stateIsActive should behave differently" to the rewrite wishlist in the wiki

taylorzane commented 7 years ago

Nice, works for me. Go ahead and close this PR if that solves this issue 👍