Skip to content
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

Move API calls on community page to client side #995

Merged
merged 7 commits into from
Feb 15, 2020

Conversation

iAdramelk
Copy link
Contributor

Right now on community we fetch data from APIs inside getInitialProps it locks page render until we get all responses and also prevents us from caching renders results for this page as static html.

This PR moves these requests to the client side.

@shcheklein shcheklein temporarily deployed to dvc-landing-async-commu-rmgs1p February 14, 2020 15:56 Inactive
@shcheklein
Copy link
Member

@fabiosantoscode PTAL :)

Copy link
Contributor

@fabiosantoscode fabiosantoscode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of nitpicks and a typo. It looks good to me.

src/utils/api.js Outdated
return count
}
} catch (e) {
console.error(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope: maybe we should reject the promise when there's a non-200 response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly a question of there to handle rejections. We can do it either here or in components themselves. For now I decided that it will probably be easier to do there them increase components size.

P. S. I like your idea to transforms API calls to custom hooks, it will solve this problem and they can also have error state in them that components can use. I will try to convert them to custom hooks now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean.

But maybe a non-200 response should have the same treatment as an error. This decreases the amount of outcomes the function may have. Right now, it's [error, undefined, response]. If we treat non-200 as an error it's only [error, response].

src/components/Community/Meet/index.js Outdated Show resolved Hide resolved
src/components/Community/Meet/index.js Outdated Show resolved Hide resolved
const [issues, setIssues] = useState([])

const [isTopicsLoaded, setIsTopicsLoaded] = useState(false)
const [topics, setTopics] = useState([])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps we could use a custom hook for these API calls, to keep things consistent:

const useAPICall = (apiFn, ...args) => {
  const [loading, setLoading] = useState(false)
  const [result, setResult] = useState(null)

  useEffect(() => {
    setLoading(true)
    setResult(null)
    apiFn(...args).then(result => {
      setLoading(false)
      setResult(result)
    })
  }, [apiFn, ...args])

  return { loading, result }
}

// ...
const topics = useAPICall(getLatestIssues, /* args for API call can go here */)

topics.loading // bool
topics.result // any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'd probably would add error state here too.

@iAdramelk iAdramelk temporarily deployed to dvc-landing-async-commu-rmgs1p February 14, 2020 17:42 Inactive
@iAdramelk
Copy link
Contributor Author

@fabiosantoscode Rewrote with hooks, please check.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Minor language suggestions below.

p.s. sanity check on https://dvc-landing-async-commu-rmgs1p.herokuapp.com/community also looks good. I can see the blog, discourse, and github calls in Network tab.

src/components/Community/Learn/index.js Outdated Show resolved Hide resolved
src/components/Community/Meet/index.js Outdated Show resolved Hide resolved
src/components/Community/Meet/index.js Outdated Show resolved Hide resolved
src/utils/api.js Outdated Show resolved Hide resolved
@fabiosantoscode
Copy link
Contributor

Looks good.

The fact that we have to write so much code for this makes me sad we can't inspect promises from the outside. We would be able to read the result, error and whether it's resolved.

shcheklein and others added 2 commits February 14, 2020 17:17
@shcheklein shcheklein temporarily deployed to dvc-landing-async-commu-rmgs1p February 15, 2020 01:19 Inactive
@shcheklein
Copy link
Member

@fabiosantoscode

The fact that we have to write so much code for this makes me sad we can't inspect promises from the outside. We would be able to read the result, error and whether it's resolved.

could you elaborate please?

@fabiosantoscode
Copy link
Contributor

fabiosantoscode commented Feb 17, 2020

@shcheklein

Just an aside. But you can't inspect a promise's completion state from the outside (without .then and .catch), like promise.isFullfilled or promise.isRejected, or get its value. If that was the case, we could use that as a standard object and pass it around.

@shcheklein shcheklein deleted the async-community-requests branch March 27, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants