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

TipsPagination: Combine data fetching, fetch simultaneously #691

Merged
merged 1 commit into from Jul 10, 2020

Conversation

davidyuk
Copy link
Member

@davidyuk davidyuk commented Jul 7, 2020

No description provided.


this.tips = await Util.range(1, this.page)
.asyncMap(async (page) => Backend
.getCacheTips(this.tipSortBy, page, this.address, this.search, this.blacklist));
Copy link
Member Author

Choose a reason for hiding this comment

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

@thepiwo Was it intentional to make these requests one by one instead of doing them at the same time? I don't think that something can go wrong if the client will request them at the same time, except it has 100 pages opened or so. Also, it can be handled by the backend allowing to accept page range.

Copy link
Collaborator

Choose a reason for hiding this comment

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

my goal here was to get the order right without much code

Copy link
Collaborator

Choose a reason for hiding this comment

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

But could probably be done using Promise all as well

@davidyuk davidyuk force-pushed the feature/refactor-tips-pagination branch from 3041799 to 76929b0 Compare July 7, 2020 03:34
@shapkarin
Copy link
Contributor

@davidyuk I open an issue #693

Copy link
Contributor

@mradkov mradkov left a comment

Choose a reason for hiding this comment

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

Changes look good. @davidyuk can you resolve conflicts?

@@ -28,12 +28,11 @@
</template>

<script>
import { get, isEmpty } from 'lodash-es';
import { get, isEmpty, times } from 'lodash-es';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer not to use lodash at all

Copy link
Member Author

Choose a reason for hiding this comment

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

why so? I'm using it in edge cases when js doesn't have something out of the box. Also lodash-es uses es6 imports so only used code will appear in the bundle. Or maybe you prefer to use ramda instead? 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a specific reason, I don't think lodash is needed really. Up to you to decide

});

this.interval = setInterval(() => this.reloadData(), 120 * 1000);
this.interval = setInterval(() => this.loadData(), 120 * 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name reloadData was consistent across components

@mradkov mradkov merged commit ac146dd into develop Jul 10, 2020
@mradkov mradkov deleted the feature/refactor-tips-pagination branch July 10, 2020 08:32
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.

None yet

4 participants