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

Improve internal route and parameter parsing #330

Merged
merged 32 commits into from
Oct 9, 2020
Merged

Conversation

bakerkretzmar
Copy link
Collaborator

@bakerkretzmar bakerkretzmar commented Sep 25, 2020

This PR rewrites most of Ziggy's JavaScript core with two main goals:

  1. Take advantage of the bindings property added in Add full support for implicit route model binding #315 to more reliably handle complex nested parameters and route model binding.
  2. Don't mutate Ziggy's own internal state while it's parsing/compiling a URL—we used to delete properties from this.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:

  • Parameters are immediately parsed into an object, with parameter names as keys and plain strings or integers as values. This makes merging in default parameter values and compiling the final URL much simpler.
  • Route template 'segments' are parsed into an ordered array of objects, which makes them easier to access repeatedly and to perform merge-like operations on with their parameter values.
  • There is a new 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 to current() 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:

  • the publicly available name, absolute, ziggy, urlBuilder, template, urlParams, queryParams, and hydrated properties on the Router class
  • the publicly available normalizeParams(), hydrateUrl(), matchUrl(), constructQuery(), extractParams(), parse(), and trimParam() methods on the Router class

And deprecates:

  • with() in favour of just passing parameters to the route() function
  • check() in favour of has() for consistency with Laravel

And 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:

  • Update Changelog

…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'
@bakerkretzmar bakerkretzmar added enhancement in progress We're working on it labels Sep 25, 2020
@bakerkretzmar bakerkretzmar added this to the v1.0 milestone Sep 25, 2020
Copy link
Member

@mattstauffer mattstauffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freaking fantastic work

src/js/route.js Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
src/js/route.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement in progress We're working on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there any way to make route().current() also match params?
2 participants