Skip to content
This repository has been archived by the owner on Nov 11, 2023. It is now read-only.

Add a clean way to consolidate data in Poll #43

Merged
merged 2 commits into from
Sep 12, 2018
Merged

Conversation

fabien0102
Copy link
Contributor

@fabien0102 fabien0102 commented Sep 12, 2018

Why

Our server send us chunk instead of a complete response on long polling (for obvious performance reasons ^^)

The idea is to have prevData available in the Poll:resolve props, so we can do:

<Poll path="" resolve={(data, prevData) => (prevData || "") + data}>
  {/* my awesome application */}
</Poll>

I also add a lot of unit tests to Poll and fix some little behaviour on my way:

  • avoid a useless call on start
  • cancel the request on component did mount

@fabien0102 fabien0102 added the enhancement New feature or request label Sep 12, 2018
@fabien0102 fabien0102 self-assigned this Sep 12, 2018
@fabien0102 fabien0102 requested a review from TejasQ September 12, 2018 07:51
@contiamo-ci
Copy link

Warnings
⚠️

❗ Big PR

Generated by 🚫 dangerJS

this.keepPolling = false;
this.setState(() => ({ polling: false, finished: true })); // let everyone know we're done here.
};

public componentDidMount() {
const { path, lazy } = this.props;

if (!path) {
if (path === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

But path is required in the interface 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? Why can we not poll on the base? It's works for Get so…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep the path mandatory to avoid mistake, so we need to path="" if we want this case (a bit stupid but explicit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I may have your comment, it's for javascript users ^^ (this case is not possible in typescript of course)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

@TejasQ TejasQ merged commit c3bfe77 into next Sep 12, 2018
@TejasQ TejasQ deleted the feat-poll-strategy branch September 12, 2018 11:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants