Move path/patterns to common/routes. Export get*Path functions for correct value#39833
Merged
jfsiii merged 4 commits intoelastic:feature-integrations-managerfrom Jul 1, 2019
jfsiii:named-route-improvements
Merged
Move path/patterns to common/routes. Export get*Path functions for correct value#39833jfsiii merged 4 commits intoelastic:feature-integrations-managerfrom jfsiii:named-route-improvements
jfsiii merged 4 commits intoelastic:feature-integrations-managerfrom
jfsiii:named-route-improvements
Conversation
jasonrhodes
reviewed
Jun 28, 2019
jfsiii
commented
Jun 28, 2019
|
|
||
| interface RouteDef { | ||
| path: string; | ||
| generatePath: (replacement?: string) => string; |
Contributor
Author
There was a problem hiding this comment.
I should note this is “works for the 3 cases we have now”-level code. We’ll come back to this and change the shape as required.
jasonrhodes
reviewed
Jun 29, 2019
jasonrhodes
reviewed
Jun 29, 2019
jasonrhodes
reviewed
Jun 29, 2019
jasonrhodes
requested changes
Jun 29, 2019
Member
jasonrhodes
left a comment
There was a problem hiding this comment.
Looks good, just a few changes/comments to work through. My overall reaction is this may be a bit more than what we need right now to get something simple off the ground, but I see the advantages too.
Simple. Works for now. Easy to delete later.
Contributor
Author
|
@jasonrhodes mind taking another look? I made it much closer to what we’re currently doing, but still moved knowledge/work out of the views. |
Contributor
💚 Build Succeeded |
jasonrhodes
approved these changes
Jul 1, 2019
Member
jasonrhodes
left a comment
There was a problem hiding this comment.
Looks great, thanks for simplifying. 👍
This file contains hidden or 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
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.
Summary
TL;DR: the client-side data fetching functions changed.
export async function getIntegrationsList(): Promise<IntegrationList> { - const path = API_INTEGRATIONS_LIST; + const path = getListPath(); const list: IntegrationList = await _fetch(path); return list; } export async function getIntegrationInfoByKey(pkgkey: string): Promise<IntegrationInfo> { - const path = API_INTEGRATIONS_INFO.replace('{pkgkey}', pkgkey); + const path = getInfoPath(pkgkey); const info: IntegrationInfo = await _fetch(path); return info;Problem
The client needs to know the location of certain routes (e.g. package list & detail APIs). The server clearly needs to know about these to create the handlers. How can we define route names & paths in one location without making the
client&serverworlds collide?Proposal
Store the path/pattern that the server router uses from in
common/routes.The server imports the paths from
common/routesand defines its route handlers inserver/routes.The client calls
get*Path, with any parameters to substitute, and that function returns the correct path.