-
Notifications
You must be signed in to change notification settings - Fork 247
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
Improve internal route and parameter parsing #330
Merged
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
…descriptive objects: - immediately extract the name of every parameter present in the route template - check what the type of the passed parameters *is* (instead of what it *isn't*) so that we can wrap strings and integers in an array and completely ignore objects - pre-fill default parameters immediately - remove the numericParamIndices flag and conditionals that check for it - remove conditionals that depend on missing parameter key names, since there now shouldn't ever be any
- use route model binding keys more frequently now that they're always available - use type checks to return early for non-object parameter values - don't modify the parameters object with `delete` during matching - normalize single object parameters properly
- remove unreachable error thrown if the provided `name` is undefined (called from UrlBuilder which was only instantiated at all if `name` was set) - return early from hydrateUrl if there are no template segments to replace - error as early as possible on non-existent routes - remove check for pre-existing hydrated URL (this never happened) - formatting
…atching params - Remove matchUrl method (undocumented, only used internally, and pretty much useless on its own) - Refactor some internal methods to accept a route so they can be used 'statically' (in any context) - Match passed params partially against current route - Add option to match passed params exactly when necessary - Remove normalizeParams method and replace with internal _parseParameters() - Formatting and renaming things
- extract new substituteBindings helper and replace switch statements - extract new dehydrate helper - add comments and jsdocs - improve error messages - rename and deprecate things, mostly maintaining backwards compatibility - add class properties - rename internal configuration object from 'ziggy' to 'config'
This reverts commit c22342c.
mattstauffer
requested changes
Sep 28, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Freaking fantastic work
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.
This PR rewrites most of Ziggy's JavaScript core with two main goals:
bindings
property added in Add full support for implicit route model binding #315 to more reliably handle complex nested parameters and route model binding.delete
properties fromthis.urlParams
as we handled them, which constrained the order they could be handled in and caused other headaches.To accomplish this, several things are now handled quite differently internally:
Route
class that encapsulates functionality related to handling one specific route, as opposed to some or all of them.This PR also adds one hot new feature: the ability to pass a second argument to
current()
with an object containing parameters to check in addition to the route name. If that sounds familiar it's because I copied and pasted that sentence from #314, which also added that feature, but this time I didn't half-ass it and so the caveat in that PR no longer applies—now, the object passed tocurrent()
will be partially matched against the current URL, so you can pass a subset of parameters and only the ones you pass will be checked. This PR therefore closes #314 and closes #211.This PR removes:
name
,absolute
,ziggy
,urlBuilder
,template
,urlParams
,queryParams
, andhydrated
properties on theRouter
classnormalizeParams()
,hydrateUrl()
,matchUrl()
,constructQuery()
,extractParams()
,parse()
, andtrimParam()
methods on theRouter
classAnd deprecates:
with()
in favour of just passing parameters to theroute()
functioncheck()
in favour ofhas()
for consistency with LaravelAnd uses microbundle's property mangling to restrict internal APIs.
Yes, that's a lot to remove. None of these APIs were documented, and most of them were only used or intended to be used internally. Removing them and replacing their functionality with internal properties and methods (hidden by mangling them as described above) gives us a lot more flexibility to tweak their implementation later and makes Ziggy's intended uses and what its public APIs are much clearer. Most of the functionality of the removed methods that do more than simple string parsing is still available in other ways, and most of the information contained in the removed properties is available already in the global
Ziggy
configuration object.Todos before merging: