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

Feat/frontend event trigger #361

Open
wants to merge 4 commits into
base: master
from

Conversation

@topocount
Copy link
Contributor

commented Aug 20, 2019

Changes

This submission resolves #119 and restores the ability to communicate with the script reducer from the frontend, but in an officially documented fashion.

  • I have updated the associated documentation with my changes
topocount added 3 commits Aug 16, 2019

@auto-assign auto-assign bot requested review from 2color and sohkai Aug 20, 2019

@osarrouy

This comment has been minimized.

Copy link

commented Aug 27, 2019

Hey all!

Any idea when this PR could be merged ? It would really help us to clean how state is handled in fundraising.

Because of the tap lowering the pool reserve every block we basically need to re-call balance on the reserve every 15 seconds and then update a bunch of state entry based on that ... It would be way cleaner if we could trigger the state update to go through the background script and app reducer ...

@sohkai
Copy link
Member

left a comment

A couple of high-level items to discuss before going farther into more details.

I know Fundraising needs this feature a bit more urgently than I'd like (the issue's been open for more than a year, so there's been ample time to have signalled this requirement 😉), so we may have to release early and make some choices that will let us work towards a final implementation without backwards compatibility issues.

/**
* subscribe to an observable that emits events created by the frontend event triggers
*/
frontendTriggers () {

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 1, 2019

Member

Perhaps we can rename this to just triggers, or perhaps observeTriggers.

I'm also not sure about thinking about this as a solely frontend -> background script type of action; my original thought was to let this be generic and bi-directional. However I hadn't considered how the sending side would ignore its own messages, which might be tricky (and perhaps not worthwhile) to solve.

@@ -231,6 +231,7 @@ export default class Aragon {
this.initAppIdentifiers()
this.initNetwork()
this.initNotifications()
this.trigger = new Subject()

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 1, 2019

Member

Rather than having a global "trigger" message bus, I've been thinking more and more about having application-specific contexts to manage this better (ie. anything that's not really relevant for a client to subscribe to).

A few things fit:

  • App triggers
  • App state (the localstorage / indexeddb caching will be turned into a side effect of the state soon)
  • Perhaps app paths

And maybe more.

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 1, 2019

Member

An initial version of this context has been implemented in #352 (implementation).

@sohkai sohkai referenced this pull request Sep 1, 2019
1 of 1 task complete
@topocount

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

I know Fundraising needs this feature a bit more urgently than I'd like (the issue's been open for more than a year, so there's been ample time to have signalled this requirement wink), so we may have to release early and make some choices that will let us work towards a final implementation without backwards compatibility issues.

While it may be true that this issue has been open for over a year, a workaround was available (and utilized by Rewards and Projects) until #294 was implemented, so this PR only became urgently needed a couple months ago.

Rather than having a global "trigger" message bus, I've been thinking more and more about having application-specific contexts to manage this better (ie. anything that's not really relevant for a client to subscribe to).

We can discuss this more on the all-devs call tomorrow, but I was initially planning to implement this entirely within the aragonAPI and bypass the wrapper. However, I didn't see any patterns in aragonAPI where the flow was contained completely outside of the wrapper, so I was hesitant to implement something that lived entirely within aragonAPI Happy to go back to this implementation if you think it makes more sense.

App state (the localstorage / indexeddb caching will be turned into a side effect of the state soon)

I'm curious to learn more about this!

@sohkai

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

We can discuss this more on the all-devs call tomorrow, but I was initially planning to implement this entirely within the aragonAPI and bypass the wrapper.

There is not really a way to do this; aragonAPI without a wrapper in-between is basically two dead ends. The RPC flow is designed to always go through the wrapper module (unless we start associating the running instances of an app together, but then a whole other set of APIs need to be built).

@chadoh chadoh referenced this pull request Sep 13, 2019
12 of 12 tasks complete
@chadoh chadoh referenced this pull request Sep 20, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.