This repository has been archived by the owner on Nov 11, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 109
**Feature:** Add support for validators that are promise-based or throw errors #72
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
This is a simple live demo of `restul-react` components primarily used to test features. Run it with `npm run example`. | ||
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
<div id="app"></div> | ||
<script src="index.tsx"></script> |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import * as React from "react"; | ||
import { render } from "react-dom"; | ||
|
||
import { Get, RestfulProvider } from "../src"; | ||
|
||
const wait = timeout => | ||
new Promise((resolve, reject) => { | ||
setTimeout(() => { | ||
resolve(); | ||
}, timeout); | ||
}); | ||
|
||
const App: React.SFC<{}> = props => ( | ||
<RestfulProvider base="https://dog.ceo"> | ||
<Get path="/api/breeds/list/all" resolve={res => wait(1500).then(() => Promise.resolve(Object.keys(res.message)))}> | ||
{(breeds, { loading, error }) => { | ||
if (loading) { | ||
return "loading.."; | ||
} | ||
if (error) { | ||
return <code>{JSON.stringify(error)}</code>; | ||
} | ||
if (breeds) { | ||
return <ul>{breeds.map((breed, breedIndex) => <li key={breedIndex}>{breed}</li>)}</ul>; | ||
} | ||
return null; | ||
}} | ||
</Get> | ||
</RestfulProvider> | ||
); | ||
|
||
render(<App />, document.querySelector("#app")); |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Shared types across exported components and utils | ||
// | ||
/** | ||
* A function that resolves returned data from | ||
* a fetch call. | ||
*/ | ||
export type ResolveFunction<T> = ((data: any) => T) | ((data: any) => Promise<T>); | ||
|
||
export interface GetDataError<TError> { | ||
message: string; | ||
data: TError | string; | ||
} |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { GetDataError, ResolveFunction } from "../types"; | ||
|
||
export const resolveData = async <TData, TError>({ | ||
data, | ||
resolve, | ||
}: { | ||
data: any; | ||
resolve?: ResolveFunction<TData>; | ||
}): Promise<{ data: TData | null; error: GetDataError<TError> | null }> => { | ||
let resolvedData: TData | null = null; | ||
let resolveError: GetDataError<TError> | null = null; | ||
try { | ||
if (resolve) { | ||
const resolvedDataOrPromise: TData | Promise<TData> = resolve(data); | ||
resolvedData = (resolvedDataOrPromise as { then?: any }).then | ||
? ((await resolvedDataOrPromise) as TData) | ||
: (resolvedDataOrPromise as TData); | ||
} else { | ||
resolvedData = data; | ||
} | ||
} catch (err) { | ||
resolvedData = null; | ||
resolveError = { | ||
message: "RESOLVE_ERROR", | ||
data: JSON.stringify(err), | ||
}; | ||
} | ||
return { | ||
data: resolvedData, | ||
error: resolveError, | ||
}; | ||
}; |
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.
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.
We already have codesandbox examples and unit tests. If you can avoid to introduce something else to maintain… And I'm sorry but this is not replacing unit tests. I can help you to add the correct unit tests for this feature if needed but please remove this.
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 is just a
dev-server
kind of like in@operational/components
. It doesn't really add much bloat to the library. It's just an on-device, in-browser player like GraphiQL. Ideally, we don't maintain this but make sure our features work in a browser context.I think the tests:
but, there is no guarantee that they work 100% in a browser environment with calls to real APIs. This will serve as a check we do during development to make sure things work before pushing.
I don't really see any major costs to having this around. Do you?
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.
@peterszerzo he does have a point though – we should ideally add unit tests for the
resolve
behavior as well. I'm happy to add these if you like.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 see this more as an integration test as opposed to a unit test - it makes sure that a real request can be made, goes through the state management, and renders something in the end. Which is something I am not confident a unit test suite would catch, especially after a feature this complicated.
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 don't dispute the need for improving the tests for the new
resolve
- I just don't feel comfortable shipping this feature without validating it with an end-to-end example. This could even grow to be like the visual test cases suite in https://github.com/contiamo/operational-visualizationsThere 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.
And at this library stage, I'm not agree with you, we must have unit tests for every features. Without tests, we can't be sure that the functionally works as expected, it's hard to maintained and everybody can broke the feature by mistake (and by everybody, I include myself of course).
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'm maybe a bit strict on this stack, but all our product depends on this library 😉
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.
As long as we have you on the team, I am confident that this will never happen. 😉 Let's also look at making the examples richer and exportable to CodeSandbox (thanks, by the way, @CompuIves) at a later stage.
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.
@fabien0102 we don't have to merge until we have a satisfactory test suite, and that would have been the case whether we had the example server or not. Both methods of testing are good for reliability and for catching issues that can bubble up in dependent projects - there is no reason to jump against one of them.
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.
@peterszerzo I agree that both methods of testing are good in the context of a PR, but our goal is to have a maintainable product in the long term. For this purpose, unit tests ensure a non regression in the future, it's automated in the CI, etc etc... So no, both methods of testing are not equal in this case.