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

Support monitors reducers #87

Closed
zalmoxisus opened this issue Apr 4, 2016 · 12 comments
Closed

Support monitors reducers #87

zalmoxisus opened this issue Apr 4, 2016 · 12 comments

Comments

@zalmoxisus
Copy link
Owner

Due to Chrome restrictions, an extension cannot communicate directly with the page script, so the only thing the injecting script does is adding instrument enhancer and relaying all the changes to the extension's background script, where we have all the orchestration.

Before we open a monitor window, no data are relayed to the extension and no monitors are rendered. In this way the extension wouldn't have signifiant impact on the app while not using it.

We need somehow to consume monitors' reducers on the app side without rendering the monitors. Also we should keep the ability to add / remove monitors on the fly.

@khankuan
Copy link

khankuan commented Apr 9, 2016

Hmm, do you mean that because the monitor may not be launched at the start of the app, the monitor reducer might not work as expected?

@zalmoxisus
Copy link
Owner Author

@khankuan, we don't render monitors inside the user's app, so they are not present there at all. Applying all available monitors and hiding the rendered html just to access their reducers wouldn't be a reasonable solution I think.

@khankuan
Copy link

khankuan commented Apr 9, 2016

Hmm not sure if I got the flow correct. Could we use monitor reducers the same way as the current reducer?

@zalmoxisus
Copy link
Owner Author

Actually, I looked into the shift toggle implementation, and you don't need any reducers applied on the user's app side, everything should be done on the remote side monitor. We should just allow sending the changes from there to the app, which should be an easy fix in remotedev-app. I'll look into it.

The current issue is related only to filterable-log-monitor, which is for now the only monitor that we cannot support here.

To understand the flow, take a look at remote-redux-devtools. It's the same principle and it's agnostic to any monitors, because they are applied outside (it can be even on the other device).

@khankuan
Copy link

Hmm, i checked out filterable-log-monitor. Which part of it required code on user's app side?

Cheers

zalmoxisus added a commit to zalmoxisus/remotedev-app that referenced this issue Apr 12, 2016
@zalmoxisus
Copy link
Owner Author

@khankuan, sorry for the late response.

actionIdToDatumMap should be generated on the user's app side and sent with the liftedState. We could include its monitor as well to get it work, the issue is only to do it only when needed, which can be addressed later if we decide to support this monitor.

Regarding the shift toggle implementation, I tried your fork and it works great now (except the issue I commented).

Give it a try and feel free to reopen the issue if you have any troubles.

@khankuan
Copy link

@zalmoxisus hmm, I updated to the latest version but it seems like this.props.dispatch does not dispatch actions with type @...

@zalmoxisus
Copy link
Owner Author

@khankuan make sure to update remotedev-app as well, it should be at least 0.3.1 (as specified on package.json) to support this change.

@khankuan
Copy link

I'm running on master and also tried updating to 0.3.2 but seems the same :/

@zalmoxisus
Copy link
Owner Author

@khankuan, strange. Just checked, replaced redux-devtools-log-montor lib with that generated from your fork, and it works well except for the error I described above that makes it to work only first time.

@khankuan
Copy link

I've fix the issue you've described here :)
gaearon/redux-devtools-log-monitor@5b0c7bb

@zalmoxisus
Copy link
Owner Author

👍 Tested and it works now without any issues.

Check inside node_modules/remotedev-app whether this change is present and to log what is being dispatched. Also check if the extension receives this and dispatch inside the app.

sports-fan added a commit to sports-fan/remotedev-app that referenced this issue Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants