Skip to content
This repository has been archived by the owner on Jul 14, 2022. It is now read-only.

not getting generations which are already loaded #76

Merged
merged 10 commits into from
Nov 23, 2018

Conversation

ror-shubham
Copy link
Contributor

@ror-shubham ror-shubham commented Nov 1, 2018

No description provided.

Copy link
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

This version of store was implemented in a way that every block was loaded once. I still don't understand why @sadiqevani has replaced it with the less efficient approach (block should be loaded even if it was loaded before).

The code in this PR is too complex, I propose to implement it as it was before.


if (!generations.length) {
return state.generations
let height = await dispatch('height')
Copy link
Member

Choose a reason for hiding this comment

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

Use const instead of let where it is possible.

@ror-shubham
Copy link
Contributor Author

@davidyuk Please have a look at the latest commits.

Copy link
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

I propose firstly to merge #92, #122 and then rebase it on top of master.

blocks: [],
generations: []
generations: {},
hash_to_height: {}
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we are using camelCase.

@ror-shubham
Copy link
Contributor Author

@davidyuk I rebased this on top of other PRs and made the suggested changes. Please review again.

Copy link
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

In general it looks ok, probably later I will propose some improvements.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants