Release@next [] #219
Release@next [] #219
Conversation
…r-components into development
…r-components into development
… and fetchById methods (#206) * feat: add useFetchBySlug, useFetchById hooks and plain js fetchBySlug and fetchById methods * chore: fix test * chore: renaming isFetching to isLoading * chore: fix tests * chore: changing types name to match style guide * chore: lint fixes * chore: update readme with example of useFetchBySlug
| try { | ||
| await fetchExperienceEntry({ | ||
| // @ts-expect-error intentionally setting it to undefined | ||
| client: undefined, | ||
| experienceTypeId: 'books', | ||
| locale: 'en-US', | ||
| identifier: { slug: 'slug' }, | ||
| }); | ||
| } catch (e) { | ||
| expect((e as Error).message).toBe( | ||
| 'Failed to fetch experience entities. Required "client" parameter was not provided' | ||
| ); | ||
| } |
There was a problem hiding this comment.
[request change]
This test won't function correctly. If the function doesn't throw an error, we don't call expect and thus don't make the test as failed. Also expect calls should never be conditionally but also have to be called. Please have a look at the docs of toThrow (here) and rejects (here).
await expect(async () => {
await fetchExperienceEntry({
// @ts-expect-error intentionally setting it to undefined
client: undefined,
experienceTypeId: 'books',
locale: 'en-US',
identifier: { slug: 'slug' },
});
}).rejects.toThrow('Failed to fetch experience entities. Required "client" parameter was not provided');
The same goes for the other broken tests in this file.
There was a problem hiding this comment.
These tests were brought over from the other project, so they would of had the issue there as well. I'll take a look at the suggestion and improve it.
| try { | ||
| await fetchReferencedEntities({ | ||
| // @ts-expect-error intentionally setting it to undefined | ||
| client: undefined, | ||
| experienceEntry: compositionEntry, | ||
| locale: 'en-US', | ||
| }); | ||
| } catch (e) { | ||
| expect((e as Error).message).toBe( | ||
| 'Failed to fetch experience entities. Required "client" parameter was not provided' | ||
| ); | ||
| } |
There was a problem hiding this comment.
[request change]
Same feedback as for the tets in fetchExperienceEntry.spec.ts that try/catch clauses are not working for jest tests.
There was a problem hiding this comment.
[nitpick]
There is a typo in this file name. It should be feetchExperienceEntry.spec.ts.
| await waitFor(() => { | ||
| expect(result.current).toBeDefined(); | ||
| expect(result.current.isLoading).toBe(true); | ||
| }); |
There was a problem hiding this comment.
[nitpick]
waitFor is not meant to be used with multiple assertions and can lead to unwanted issues:
| await waitFor(() => { | |
| expect(result.current).toBeDefined(); | |
| expect(result.current.isLoading).toBe(true); | |
| }); | |
| await waitFor(() => expect(result.current?.isLoading).toBe(true)); |
See here for more explanation on this: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#having-multiple-assertions-in-a-single-waitfor-callback
| }, | ||
| }); | ||
|
|
||
| await waitFor(() => { |
There was a problem hiding this comment.
[nitpick]
Same problem as with in the comment above. Especially this waitFor is a very brittle check that can easily break.
| }, | ||
| }); | ||
|
|
||
| await waitFor(() => { |
There was a problem hiding this comment.
[nitpick]
Same feedback about invalid multiple assertions in waitFor
There was a problem hiding this comment.
This is multiple times happening in this test file.
| ); | ||
| } catch (e) { | ||
| expect((e as Error).message).toBe( |
There was a problem hiding this comment.
[nitpick]
Same feedback as above that try/catch doesn't work in jest tests as intended. Use toThrow instead.
| localeCode, | ||
| }: useClientSideExperienceFetchersProps) => { | ||
| /** | ||
| * @deprecated please use `useFetchBySlug` or `useFetchById` hooks instead |
There was a problem hiding this comment.
I guess we can remove this one as well as part of the major release?
There was a problem hiding this comment.
This will be the first release where it is deprecated. I'd usually wait at least one release to let users move over before it is removed, but since this is alpha its not as big of a deal. Might be good to chat about it as a whole.
There was a problem hiding this comment.
Thanks for providing this PR, re-structuring our logic and merging all these improvements :)
I initially requested changes on a few nitpicks and not working unit tests but I guess it's better to merge this one first and then create additional PRs based on those as this is not blocking.
Note: at least we might wanna consider removing deprecated methods as part of this major release.
Co-authored-by: Thomas Kellermeier <Chaoste@users.noreply.github.com>
Co-authored-by: Thomas Kellermeier <Chaoste@users.noreply.github.com>
…onfigure ctfl env vars via localStorage [] (#215) * chore: wip * chore: wip * feat(test-app): add ability to configure ctfl env vars via localStorage * chore(visual-editor-app): removing visual-editor-app package * chore: package-lock fix
No description provided.