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

Cache fetched Dapps #3804

Merged
merged 6 commits into from
Dec 11, 2016
Merged

Cache fetched Dapps #3804

merged 6 commits into from
Dec 11, 2016

Conversation

ngotchac
Copy link
Contributor

We shouldn't need to reload dapps on each visit to the dapps view/page

This PR adds a dappsFetcher that caches current dapps and their meta data.
It listens to the dappReg events to update the image/meta-data if needed.

This prevents a high loading time on visiting the Dapps tab.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 10, 2016
@@ -62,7 +62,7 @@ export default handleActions({
signerSuccessConfirmRequest (state, action) {
const { id, txHash } = action.payload;
const confirmed = Object.assign(
state.pending.find(p => p.id === id),
state.pending.find(p => p.id === id) || { id },
Copy link
Contributor

@jacogr jacogr Dec 10, 2016

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be. Just saw this issue once, so I guess it won't hurt to add this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think it solves that issue. Still unsure exactly why it happens, it should have all the info. (Just a nightmare to replicate - I have tried.)

const BUILTIN_APPS_KEY = 'BUILTIN_APPS_KEY';
let dappsFetcherInstance = null;

export default class DappsFetcher extends EventEmitter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is not just a store that gets passed (or is a single instance). I prefer to stick to the same patterns for consistency instead of adding yet another way of doing things.

Copy link
Contributor

Choose a reason for hiding this comment

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

So looking at where it was split from - rather use the store as a global instance (one existing), get it to update, share it as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was more to have a store for data (that gets persisted throughout the session) and one for the UI, which is a MobX store with observers & all. The idea is that we might not want to preserve MobX stores throughout the app since they don't old any valuable data if the component is not mounted.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it is actually a "global" store - gets used in multiple places (should not have been re-instantiated) and the data is store anyway now.

Let's follow one pattern, not multiples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So where does this store should live ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, probably keep it in dapps (like the fetcher was), longer term we can probably s/redux/stores and put it in there since it deals with all the global stores.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 278fed9 on ng-better-loadings into ** on master**.

@ngotchac ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 11, 2016
@ngotchac
Copy link
Contributor Author

@jacogr Ok so modified a bit how it works. Fetching dapps logic is now in util/dapps.js (stateless methods), and the Dapps Store is now a singleton that get instantiated once.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 11, 2016
@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

Cool, will play.

(As an aside, we definitely need a better place to put dapps.js/wallet.js/etc. - that folder is going to get out of control and it "feels" wrong just dumping all in there. No better ideas at this point and it is not a crisis, just something to keep in mind)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 85.587% when pulling 8ded113 on ng-better-loadings into 6724f57 on master.

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

Looks good, however just do the "Ethcore" -> "Parity Technologies" header updates.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 11, 2016
@jacogr jacogr merged commit 4dbfcf2 into master Dec 11, 2016
@jacogr jacogr deleted the ng-better-loadings branch December 11, 2016 20:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants