-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Support Route optional parameters/segments #10212
Conversation
a5f9627
to
ba9a297
Compare
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.
Adding review notes
} else if (this.baseURL[0] === '/' && this.rootURL[0] === '/') { | ||
// if baseURL and rootURL both start with a slash | ||
// ... remove trailing slash from baseURL if it exists | ||
this.baseURL = this.baseURL.replace(/\/$/, ''); |
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.
See if we can precompile and hoist these trailing slash regex's
ui/packages/consul-ui/app/components/consul/upstream/list/index.hbs
Outdated
Show resolved
Hide resolved
return url | ||
.replace(new RegExp(`^${this.baseURL}(?=/|$)`), '') | ||
.replace(new RegExp(`^${this.rootURL}(?=/|$)`), '') | ||
.replace(/\/\//g, '/'); // remove extra slashes |
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.
Another pre-compilable regex
} | ||
if (typeof hash.partition !== 'undefined') { | ||
hash.partition = `-${hash.partition}`; | ||
} |
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.
Comment this out for now
ok @kaxcode I made those couple of tweaks we discussed yesterday, so this is ready again now I think. Quick note on the swap of |
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.
I took another look and I believe I understand all the changes. LGTM - just needs a changelog.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/374912. |
🍒✅ Cherry pick of commit 2e4c9f5 onto |
Moves our URLs with 'optional namespace segment' into a separately abstracted 'optional URL segment' feature
This felt like a fairly significant change, so I did a fairly large write up:
This PR moves our URLs with 'optional namespace segment' into a separately abstracted 'optional URL segment' feature.
When Consul added namespace support we chose to detect 'optional' namespace parameters in the UIs URL, recognising a 'namespace' parameter via the presence of a
~
character at the beginning of the segment (a character that is not supported within DNS). This meant we could support URLs with the following format:Support for this led to various scattered areas in the codebase that had logic to support this, and also meant that there were two separate Routes (that we automatically created) for both of the above URLs just to support an 'optional parameter'.
In order to support our upcoming admin partition feature, we would like to use the same approach to allow an optional segment for admin partitions again by using a prefix that is not allowed via DNS
-
.Instead of repeating what we had done for namespaces by duplicating and making the necessary code more complex, we decided to refactor the feature to only be about 'optional URL segments' rather than anything to do with namespaces or admin partitions.
FSM Location
Ember's Router/Routing and related classes/services are rather complex and originate from the very beginning of ember, but they use the notion of 'Locations' to provide different ways of dealing with URL changes within your app. Locations are far more lightweight and easier to work with than working directly with Ember's routing classes, so we chose to write a set of custom locations in order to extract this feature into a more isolated approach.
Much of the work here is based on Ember's
history
location, and we've added 3 custom locations:fsm
: Based on the idea that routing could be seen as a finite state machine we've created a base FSM class which copies the interface of the browsers native History API and create a very simple custom Locationfsm-with-optional
: This builds on our baseFSMLocation
to provide support for optional URL parameters throughout our application. We currently add our namespace support (and eventually our admin partition support) in here - further work will move this project/application specific code into one of our configuration files.fsm-with-optional-test
: Following the relationship between ember'shistory
andnone
locations we add an equivalentnone
location here specifically for testing.Whilst working on the above I noticed that the way that ember instantiates the initial route for your application differs between when you are running your app and when you are testing your app. Therefore there are come changes here so that how the initial route set is no longer different between prod/dev and test. As we do a lot of injection here, this was relatively straightforwards to do without too much churn. I also took the opportunity here to replace some jQuery-ness taken from ember-cli-page-object and replace it with what we already use in our app for query parameter processing.
These locations are now the centralized place to deal with anything URL related, therefore
href-to
now gathers what it needs in order to print URLs from the location also.href-mut
As its clearer now how our application deals with URLs we also removed our unfortunately named
href-mut
helper and gavehref-to
the ability to give you URLs from the same URL you are already on, but with different parameters. We can do this now be specifying.
(meaning current URL) as the first argument (the routeName) tohref-to
. At this point we felt that it was time to remove theember-href-to
plugin. We still use a little code fromember-href-to
and that will probably be reduced further at a later date. We removed the last instance we had of ember's oldobserver
here also, so that's one less deprecation from our list.optionalParams()
Ideally we would have liked to have used
paramsFor
and the variousparams
arguments in model hooks to access these 'optional' parameters. They are just 'params' just like all our other parameters in URLs, they just happen to be optional. Unfortunately this proved to be a little difficult to integrate into ember, so we (hopefully temporarily) went with a newoptionalParams
method that is available on all routes.optionalParams
returns a hash/object of the configured optionalParams, defaulting each of them to an empty string (this decision was based on the experience of what we needed for our namespace support and felt like it made sense)Overall our application code is much slimmer and our namespace/admin partition support is now not really specific to our application, and we no longer duplicate all of our Routes to cope for the presence of an extra parameter. The usage of 'special URL segment prefix characters' like
~
are now removed from the application as a whole, and longer term will (hopefully) just be part of a line of configuration in our routing configuration.