Added Constraints #77
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Can't parse state separately to data because the data constraints might mean that need to re-match route against a different state
The handler has to check the constraints and carry on matching if they fail. So have to parse and validate all data in the Handler
Added validate function to State to check constraints. If this returns false then Handler tries to match again from this route onwards
Don't validate constraints when building Hyperlinks, only when parsing, because checking constraints can be expensive. Also, what can't do anything if it's wrong? Up to developer to protect data getting into Hyperlinks
Constraints only run for matching not building
Could have two states with same route 'a' with crumb trail turned on on
first route. Might pass { crumb: 'hello'} to the second state where the
crumb key is used but it's not a crumb trail. Then parsing Url of
'/a?crumb=hello' would cause problem because it would match first route
but wouldn't be a valid crumb trail. So need to move crumb trail parsing
into the navigation data parsing so can submit it to validation and
continue matching if validation fails.
Don't need an onError handler for constraints work. Only need to continue on error if more routes to check. So, put back the current rethrow that was removed when parseLink call skipped
State A and B can have same route but with different data. Param x could be number on A and string on B. So can get all kinds of 'valid' errors (State B could have trackTypes off and x could be string with _ This would cause splitting error when matching against A). So wrapped whole data parsing in try catch and continue matching next route if error encountered. No point reporting details errors like invalid number because it might be invalid number for State A but match State B. Or could be different errors for different States. Only set a crumb key if trackCrumbTrail is true. Otherwise end up parsing crumbs on States where trackCrumbTrail is false and stops those states using the crumb key for different data
Made StateHandler and NavigationDataManager class instances so they could own Router and ConverterFactory instances. Simplified the method parameters because don't have to pass router and converter from StateNavigator down
Need to know why the match failed, not just that the Url is invalid. At the moment only showing the last error message - there might be more than one now that constraints are around because the match continues after a match and error. Removed the global error handler in History Manager. Better to wrap start in a try catch and replace the Url. That should be only way to get an error. The global error handler is too crude because no idea where it came from. Probably add error handlers to the plugins so can handle at the Link level because can then act on the error
Don't want to throw error in set context because context would be half set and lead to odd behaviour. Move all error scenarios into link parse routine where they can be caught and handled
Tested the change and added extra crumb trail key test because there weren't any in navigation data tests
Doesn't seem possible to have a blank back url. The state handler generates the back url by parsing the crumbless url and then building url from this data and crumbs. If the parse of the crumbless url succeeded then adding crumbs to the data can't produce a blank url
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented constraints as per #73 (comment)
It's a breaking change because I removed route sorting. Used to sort routes so that the route {x} was moved to the bottom. Otherwise it might match Url that should really match 'ab', for example. But route sorting wasn't a proper solution because it didn't stop 'ab/{x}' matching before 'ab/cd'. With constraints, the matching always goes through routes in the order they appear because can now add a constraint data.x !== 'cd' to ensure the first route doesn't match in front of the second.