Enhance/compose paths #11
Conversation
456979c to
7005a07
Compare
|
I can do one better and clean up the lint fix. Just |
src/Get.tsx
Outdated
| componentDidMount() { | ||
| this.shouldFetchImmediately() && this.fetch(); | ||
| public static getDerivedStateFromProps(props: Pick<GetComponentProps, "wait" | "lazy">) { | ||
| return { loading: !props.lazy }; |
There was a problem hiding this comment.
You don't use wait anymore? and what is wait btw? ^^
There was a problem hiding this comment.
I tried to be clear about what wait is here.
I wonder how we can make it more clear. 🤔
Basically, if wait is set, it doesn't render anything until there is data.
| // If the path or base prop changes, refetch! | ||
| const { path, base } = this.props; | ||
| if (prevProps.path !== path || prevProps.base !== base) { | ||
| this.shouldFetchImmediately() && this.fetch(); | ||
| if (!this.props.lazy) { |
There was a problem hiding this comment.
Why not use this.state.loading here?
There was a problem hiding this comment.
Because the logic flow here is simply:
- did something change? 🤔
- does the user want to lazy load? 🤔
- DONT DO ANYTHING.
It doesn't quite depend on loading state.
There was a problem hiding this comment.
Sure, but if I'm in loading, that means that I have a request pending, so 2 (or more) requests can arrive in this case -> carefull to concurrency 💥
There was a problem hiding this comment.
Let's tackle this in a separate PR. I made an issue (#12) where we can handle this.
src/Get.tsx
Outdated
| const { base, path } = this.props; | ||
| this.setState(() => ({ error: "", loading: true })); | ||
|
|
||
| const { resolve } = this.props; | ||
| const foolProofResolve = resolve || (data => data); | ||
| const foolProofResolve = resolve || (noop => noop); |
There was a problem hiding this comment.
Technically, noop means no operation, and it's a classic function name that represent () => {}.
I think that I prefer the old data => data here ;)
Why
Currently, when using
Poll, everyGetinside it has its path appended to whateverPoll's path was. But why? Typically, a user would want to poll a specific endpoint and not have it affect subsequentGETrequests. This PR solves this case.It also removes the possibility of nesting
Pollpaths, which generally is a bad practice. Why would anyone nest Polls?