-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
chore: unify the workpad update handler #31218
Conversation
Pinging @elastic/kibana-canvas |
|
💚 Build Succeeded |
the 3 functions were pretty much the same already
cd52ece
to
3081d90
Compare
💚 Build Succeeded |
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.
Looks good, nice change!
import { workpad } from './workpad'; | ||
|
||
const routePrefix = API_ROUTE_WORKPAD; | ||
const routePrefixAssets = API_ROUTE_WORKPAD_ASSETS; | ||
const routePrefixStructures = API_ROUTE_WORKPAD_STRUCTURES; |
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.
Are the routes kept separate because route unification is tangential to what this PR is needed for, or because you have second thoughts about its desirability? (not suggesting that unification be done in this one, just wishing to know esp. if it's the latter)
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.
because route unification is tangential to what this PR is needed for
That ;)
Having a single handler makes the changes I'm planning easier, the route unification is separate, and still seems worth doing. And now that there's tests, we can just update the api path in them and confirm they still work the same way.
* test: assets and structures routes * chore: unify the workpad update handler the 3 functions were pretty much the same already
* test: assets and structures routes * chore: unify the workpad update handler the 3 functions were pretty much the same already
I'm planning to fix the workpad persist order issue we've had for a while, and unifying the update handler is going to make that a lot easier. The 3 functions were pretty much the same already, so using the same
updateWorkpad
handler for all 3 routes was a pretty easy change.Related: #29930 (which is about unifying the routes, this PR only unifies the handler for all 3 routes)