Skip to content

Move API calls on community page to client side#995

Merged
shcheklein merged 7 commits into
masterfrom
async-community-requests
Feb 15, 2020
Merged

Move API calls on community page to client side#995
shcheklein merged 7 commits into
masterfrom
async-community-requests

Conversation

@iAdramelk
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@fabiosantoscode PTAL :)

Copy link
Copy Markdown
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.

Comment thread src/utils/api.js Outdated
return count
}
} catch (e) {
console.error(e)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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].

Comment thread src/components/Community/Meet/index.js Outdated
Comment thread src/components/Community/Meet/index.js Outdated
Comment thread src/components/Community/Meet/index.js Outdated
const [issues, setIssues] = useState([])

const [isTopicsLoaded, setIsTopicsLoaded] = useState(false)
const [topics, setTopics] = useState([])
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

@fabiosantoscode Rewrote with hooks, please check.

Copy link
Copy Markdown
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.

Comment thread src/components/Community/Learn/index.js Outdated
Comment thread src/components/Community/Meet/index.js Outdated
Comment thread src/components/Community/Meet/index.js Outdated
Comment thread src/utils/api.js Outdated
@fabiosantoscode
Copy link
Copy Markdown
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.

Comment thread src/components/Community/Meet/index.js
shcheklein and others added 2 commits February 14, 2020 17:16
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
shcheklein and others added 2 commits February 14, 2020 17:17
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@shcheklein shcheklein temporarily deployed to dvc-landing-async-commu-rmgs1p February 15, 2020 01:19 Inactive
@shcheklein
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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