-
Notifications
You must be signed in to change notification settings - Fork 95
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
v2 does not work with redux-observable #285
Comments
I think my observation from yesterday may not be 100% correct. I think the solution that works for me, is to put both main & renderer listeners for the ACTION type after the createStore outside the enhancer. Just re-arranging the enhancers and middleware is not sufficient. |
@Superjo149 did you get this working? I think this might be closely related to our issue #294 |
@slapbox Yes, I seem to have been through the same rabbit-hole. I am actually currently working on a proper fix as we speak. I have found some hacks to get it working, but I am now working on a proper solution. However, it will change the API for the library significantly. |
@Superjo149 It would be great if you could share an overview of the direction of the fix. We could have the discussion on the API too - since redux-saga & redux-observable are rather big part of the ecosystem, we wouldn't want to release a final version without support for those |
@matmalkowski I was doing more research around this yesterday, and I landed on this section in the They state a few times, that if you are not using any different enhancers or middleware, you can just use the In this regard, I am taking a similar approach. I am adding a new method I actually just got it working as well. |
Edit: I wrote this before the two most previous messages, might be outdated. @Superjo149 thanks for your speedy reply! I think I'm going to be following your work because as much as I love the new API, I fear it may never be made compatible with Sagas and Observables. Is this the repo you're planning to do your work on? https://github.com/Superjo149/electron-redux/commits/alpha Is your thinking that Would you be able to share these "hacks" that you got Observables working with? I've come pretty far along this path and really want to avoid having to reverse course now. It seems like the main problem we're seeing is that new Redux actions triggered from our sagas aren't firing. (although I may have just gotten that working, if it is working, I couldn't say why) |
@slapbox I will just create a PR to this repo as soon as I am happy with it. No need to start yet another fork. That repo is just to create a PR from. With the above approach, And no, I just removed those hacks while now preparing these changes, so I will not be able to share these anymore. |
@Superjo149 So seems like the way to go might be the redux-devtools-extension way, and we can keep the current API for the no enhancers usecase, while introducing also the |
Whoops, it looks like it "works" for me because I'd forgotten I tried this lunacy of running Shockingly it seems like this works well enough for us to continue our work for now, though I am sure that something will explode soon and I'd never use this in production: middleware.push(sagaMiddleware);
enhancers.push(applyMiddleware(...middleware)); // Apply middleware
if (scope === 'renderer') {
enhancers.push(require('electron-redux').rendererStateSyncEnhancer());
}
if (scope === 'main') {
enhancers.push(require('electron-redux').mainStateSyncEnhancer());
}
enhancers.push(applyMiddleware(...middleware)); // Apply middleware AGAIN |
@matmalkowski No, indeed, that may have been over exaggerated, wasn't sure yet 1 hour ago if more changes were needed. Will send a PR your way as soon as I get things cleaned up. 🙂 |
@Superjo149 that's awesome what you've found and what you're working on, thanks so much for sharing. Looks like I have some reading to catch up on. And @matmalkowski thank you so much for all your work on this project and being committed to having both Sagas and Observables working. The rapid fire developments here are super encouraging! |
…rder (#296) * fix(#285): add composeWithStateSync to resolve issues with enhancer order * fix: resolve PR comments * chore: add type tests * docs: update README getting started and add changes to FAQ Co-authored-by: Maciej Małkowski <[email protected]>
🎉 This issue has been resolved in version 2.0.0-alpha.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat: migrate to typescript (#259) * feat: migrate to typescript This is WIP * feat: package.json cleanup * chore: remove eslint config rules * chore: more cleaning * chore: add tsc tests * chore: reorganize export structure * chore: add some basic e2e tests * buid: add GH action for PR * fix: e2e test memory leak fix * chore: vscode workspace settings * fix: update username in package.json * chore: rollup.config simplify * chore: add eslint (#268) * chore: add eslint This PR adds eslint setup to the reposiory. It doesn't fix existing errors/warnings - should get addressed in follow up Pull Requests since the codebase isn't final now anyway. Errors so far: ``` /Users/maciej.malkowski/git/github.com/klarna/electron-redux/src/syncMain.ts 18:56 error Async arrow function has no 'await' expression @typescript-eslint/require-await 46:3 error Unsafe return of an any typed value @typescript-eslint/no-unsafe-return /Users/maciej.malkowski/git/github.com/klarna/electron-redux/src/syncRenderer.ts 21:8 error Unsafe assignment of an any value @typescript-eslint/no-unsafe-assignment 61:4 error Unsafe return of an any typed value @typescript-eslint/no-unsafe-return 76:3 error Unsafe return of an any typed value @typescript-eslint/no-unsafe-return 85:27 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 93:3 error Promises must be handled appropriately or explicitly marked as ignored with the `void` operator @typescript-eslint/no-floating-promises 105:3 error Unsafe return of an any typed value @typescript-eslint/no-unsafe-return 105:32 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any /Users/maciej.malkowski/git/github.com/klarna/electron-redux/src/utils/actions.ts 15:31 warning Missing return type on function @typescript-eslint/explicit-module-boundary-types /Users/maciej.malkowski/git/github.com/klarna/electron-redux/src/utils/json.ts 6:9 error Unsafe assignment of an any value @typescript-eslint/no-unsafe-assignment 7:3 error Unsafe member access .__hydrate_type on an any value @typescript-eslint/no-unsafe-member-access 22:24 warning Missing return type on function @typescript-eslint/explicit-module-boundary-types 22:36 warning Argument 'value' should be typed with a non-any type @typescript-eslint/explicit-module-boundary-types 22:43 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 23:6 error Unsafe member access .__hydrate_type on an any value @typescript-eslint/no-unsafe-member-access 27:13 error Unsafe member access .__hydrate_type on an any value @typescript-eslint/no-unsafe-member-access 28:18 error Unsafe member access .items on an any value @typescript-eslint/no-unsafe-member-access 31:2 error Unsafe return of an any typed value @typescript-eslint/no-unsafe-return /Users/maciej.malkowski/git/github.com/klarna/electron-redux/src/utils/misc.ts 7:44 warning Missing return type on function @typescript-eslint/explicit-module-boundary-types 24:29 warning Missing return type on function @typescript-eslint/explicit-module-boundary-types 33:31 warning Missing return type on function @typescript-eslint/explicit-module-boundary-types /Users/maciej.malkowski/git/github.com/klarna/electron-redux/tests/e2e.spec.ts 21:5 error Unexpected `await` of a non-Promise (non-"Thenable") value @typescript-eslint/await-thenable 36:12 error Unexpected `await` of a non-Promise (non-"Thenable") value @typescript-eslint/await-thenable 45:12 error Unexpected `await` of a non-Promise (non-"Thenable") value @typescript-eslint/await-thenable /Users/maciej.malkowski/git/github.com/klarna/electron-redux/tests/e2e/main/index.ts 31:5 error Promises must be handled appropriately or explicitly marked as ignored with the `void` operator @typescript-eslint/no-floating-promises 31:44 error Invalid type "string | undefined" of template literal expression @typescript-eslint/restrict-template-expressions 33:5 error Promises must be handled appropriately or explicitly marked as ignored with the `void` operator @typescript-eslint/no-floating-promises 54:1 error Promises must be handled appropriately or explicitly marked as ignored with the `void` operator @typescript-eslint/no-floating-promises /Users/maciej.malkowski/git/github.com/klarna/electron-redux/tests/e2e/renderer/index.ts 9:5 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion 18:5 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion 22:5 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion 32:5 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion /Users/maciej.malkowski/git/github.com/klarna/electron-redux/tests/typescript/syncMain.ts 5:7 warning 'store' is assigned a value but never used @typescript-eslint/no-unused-vars /Users/maciej.malkowski/git/github.com/klarna/electron-redux/tests/typescript/syncRenderer.ts 5:7 warning 'store' is assigned a value but never used @typescript-eslint/no-unused-vars ✖ 35 problems (20 errors, 15 warnings) ``` * fix: fix test related type issues * chore: add prettier & format the code (#269) This PR adds the prettier code formatter with custom config, and formats existing code according to those rulles. Pre-commit hook to format staged files was also added. * feat: add Code of Conduct & bump version (#270) This pull request adds Code of Conducts and marks the change as breaking for semanric release to bump the version to v2 BREAKING CHANGE: TS rewrite * feat: add semantic-release setup for automatic deployments (#271) * feat: add semantic-release setup for automatic deployments This adds the setup for semantic-release. master/beta/alpha branch changes will get deployed to npm with version depending on the commit message * chore: add PR linter * chore: add more events tro trigger the check * fix: remove default custom serialization and make it an opt-in option (#272) Removing custom serializer as a default, since its not alligned with redux recomendation about store & serialization fix #261 * fix: reduce bundle size by limiting external dependencies (#276) I removed external dependency of `flux-standard-action` that includes entire loadash with it when imported. Instead, I re-implemented single function we are using and relied on single modules from loadash that are required. Should reduce the size significantly * feat: ignore devtools when sending actions to renderers (#275) Co-authored-by: Maciej Małkowski <[email protected]> * chore: add README & LICENSE (#281) * chore: add README & LICENSE As per title, adding initial README file & LICENSE. It will need some work for sure, I've left the missing links as TODO * chore: more badges * chore: apply suggestions from code review Co-authored-by: Burkhard Reffeling <[email protected]> * Update LICENSE.md Co-authored-by: Burkhard Reffeling <[email protected]> * feat: make renderer state initialization synchronous by default (#280) * feat: make renderer state initialization synchronous by default Implements #278 * code review comments * chore: spelling Co-authored-by: Burkhard Reffeling <[email protected]> * chore: code review comments Co-authored-by: Burkhard Reffeling <[email protected]> * docs: setup base documentation website (#277) * docussaurus initial setup * base docussaurus config * add base gh-pages deployment action * remove unnecessary job step * test changing baseurl * fix last commit * fix last commit * remove trailing slash * fix baseUrl config * add basic content based on readme * use yarn in gh-pages action Co-authored-by: Maciej Małkowski <[email protected]> * feat: add option for denyList (#274) * fix: align enhancer style with standards to wrap the middleware (#284) * fix: align enhancer style with standards to wrap the middleware * fix: align enhancer style with standards to wrap the middleware * docs: adding initial content for v2 (#290) * docs: adding initial content for v2 Adding some initial content for Version 2, including expected file tree of documentation & legacy v1 docs paragraph. * chore: apply suggestions from code review Co-authored-by: Burkhard Reffeling <[email protected]> Co-authored-by: Burkhard Reffeling <[email protected]> * docs: fixing doc build issue with missing paths (#295) Also adding PR build step to prevent this from happening again * chore: update semrel lint to run on forks * fix(#285): add composeWithStateSync to resolve issues with enhancer order (#296) * fix(#285): add composeWithStateSync to resolve issues with enhancer order * fix: resolve PR comments * chore: add type tests * docs: update README getting started and add changes to FAQ Co-authored-by: Maciej Małkowski <[email protected]> * chore: update README * chore: update README links * chore: adding basic example of usage in raw scenario + moving integration tests (#299) * chore: adding basic example of usage in raw scenario * chore: work in progress * chore: trying to add tests * chore: fixing e2e tests and making them runnable * fix package.json * chore: code review comments addressed * feat: use contextBridge in favour of requiring nodeIntegration (#300) * feat: use contextBridge in favour of requiring nodeIntegration Due to security concerns related to usage of nodeIntegration flag, according to best electron practices, renderer functions should be exposed with contextBridge. This PR does exactly that. It also changes a bit API to accomodate for this feature * feat: fixing issues with test enviroment * fix: add missing preventDoubleInitialization() check * change the scope of the contextBridge bindings to only expose high level API --------- Co-authored-by: Paul Tiedtke <[email protected]> Co-authored-by: Burkhard Reffeling <[email protected]> Co-authored-by: António Almeida <[email protected]> Co-authored-by: Jonas Snellinckx <[email protected]>
Describe the bug
This is a separate ticket to highlight the second issue I was having with V2 and
redux-observable
briefly mentioned in #260 . The issue I was having, was that for some reason I had to re-arrange my enhancer and middleware, otherwise my dispatched actions from the main process weren't picked up by the renderer process. I am not sure if this should be documented or resolved.This change is related to the API change to remove
replayActionRenderer
. Listening for and dispatching actions from the middleware instead of after the createStore has been created, like the V1 api, is causing the dispatched actions to not get picked up by the middleware. The renderer process is receiving an action, the reducer are being called because of it, but not the middleware. I think it is because when the enhancer is after the middleware in the compose function, thedispatch
in the enhancer isn't setup yet with the middleware.I see 3 solutions here. Re-introduce
replayActionRenderer
,document the necessity to have the enhancer as the first item, or fix it somehow inside the enhancer itself so that the simple API remains and the compose order is not so error-prone. But I am not sure if this is possible.To Reproduce
I may be able to provide a sample project if needed, but it will take some effort to set up.
My setup: Main process -> some action -> epic middleware -> action -> dispatched to renderer -> middleware not triggered (neither redux-logger or redux-observable)
The text was updated successfully, but these errors were encountered: