-
-
Notifications
You must be signed in to change notification settings - Fork 132
Handle error during api documentation parsing #51
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
Handle error during api documentation parsing #51
Conversation
387cb3b to
985f62d
Compare
Simperfit
left a comment
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.
thanks @mauchede that will be useful !
src/hydra/HydraAdmin.js
Outdated
| .then(api => this.setState({api})); | ||
| this.props.apiDocumentationParser(this.props.entrypoint).then( | ||
| ({api, customRoutes = []}) => | ||
| this.setState({ |
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.
this feels duplicated, is it possible to not duplicate it or it's not worth the complexity ?
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.
Instead of call setState directly, we could "normalize" the data:
this.props
.apiDocumentationParser(this.props.entrypoint)
.then(
({api, customRoutes = []}) => ({
api,
customRoutes,
hasError: false,
loaded: true,
}),
({api, customRoutes = []}) => ({
api,
customRoutes,
hasError: true,
loaded: true,
}),
)
.then(state => this.setState(state));
src/hydra/HydraAdmin.js
Outdated
| } | ||
|
|
||
| if (true === this.state.hasError) { | ||
| return <span>Impossible to retrieve API documentation.</span>; |
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.
maybe this could be a key instead of directly english ?
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.
Allowing to pass props as suggested by @Gregcop1 looks better to me.
src/hydra/HydraAdmin.js
Outdated
| render() { | ||
| if (null === this.state.api) { | ||
| if (false === this.state.loaded) { | ||
| return <span>Loading...</span>; |
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.
In order to make this more configurable and user friendly, it could be possible to pass messages (loading, error, ...) on props with default values. So my french interface can keep french messages
src/hydra/HydraAdmin.js
Outdated
| render() { | ||
| if (null === this.state.api) { | ||
| if (false === this.state.loaded) { | ||
| return <span>Loading...</span>; |
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.
you should add a className here to allow customization of the render
src/hydra/HydraAdmin.js
Outdated
| } | ||
|
|
||
| if (true === this.state.hasError) { | ||
| return <span>Impossible to retrieve API documentation.</span>; |
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.
you should add a className here to allow customization of the render
src/hydra/HydraAdmin.js
Outdated
| this.props | ||
| .apiDocumentationParser(this.props.entrypoint) | ||
| .then(api => this.setState({api})); | ||
| this.props.apiDocumentationParser(this.props.entrypoint).then( |
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.
why .then on the same line?
dunglas
left a comment
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.
Handling @Gregcop1 suggestions would be nice but 👍 , great job!
985f62d to
30c50b1
Compare
src/hydra/HydraAdmin.js
Outdated
| loaded: true, | ||
| }), | ||
| ) | ||
| .then(state => this.setState(state)); |
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.
Have you tried .then(this.setState) ?
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 tried and the following error occurred:
Uncaught (in promise) TypeError: Cannot read property 'updater' of undefined
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.
It works, if I use:
.then(this.setState.bind(this))30c50b1 to
001596f
Compare
|
Thanks @mauchede! |
|
It could be very useful to put this example in the official doc of api platform because this "https://api-platform.com/docs/admin/authentication-support" doc is wrong ;) |
This PR should fix issues #43 and #31.
This PR handles parsing errors (see my PR on dunglas/api-doc-parser):
AdminBuilder).In all cases, the user has the possibility to define some
customRoutes. These routes will be added to thecustomRoutesdefined via properties.With this PR, we can now use
api-platform/adminon a fully secured API. For example:In this example:
customRouteif parsing returned a401error. This route will redirect to theloginpage.authClientwill refresh current page when a JWT token will be stored. Refresh the page will give the possibility to parse again the documentation, but this time the JWT token will be sent.