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

v2 does not work with redux-observable #285

Closed
sneljo1 opened this issue Dec 8, 2020 · 12 comments · Fixed by #296
Closed

v2 does not work with redux-observable #285

sneljo1 opened this issue Dec 8, 2020 · 12 comments · Fixed by #296
Labels
bug 🐛 released on @alpha v2 All issues related to v2

Comments

@sneljo1
Copy link
Contributor

sneljo1 commented Dec 8, 2020

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, the dispatch 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)

@sneljo1 sneljo1 changed the title V2 doesn't work with redux-observable v2 doesn't work with redux-observable Dec 8, 2020
@sneljo1 sneljo1 changed the title v2 doesn't work with redux-observable v2 only works with redux-observable when enhancers are before middleware Dec 8, 2020
@matmalkowski matmalkowski added bug 🐛 v2 All issues related to v2 labels Dec 9, 2020
@sneljo1
Copy link
Contributor Author

sneljo1 commented Dec 9, 2020

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.

@sneljo1 sneljo1 changed the title v2 only works with redux-observable when enhancers are before middleware v2 does not work with redux-observable Dec 9, 2020
@Nantris
Copy link
Contributor

Nantris commented Feb 9, 2021

@Superjo149 did you get this working? I think this might be closely related to our issue #294

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 9, 2021

@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.

@matmalkowski
Copy link
Collaborator

@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

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 9, 2021

@matmalkowski I was doing more research around this yesterday, and I landed on this section in the redux-devtools-extension. I think this library has similar issues with the order of the enhancers/middleware. This issue slipped into electron-redux due to the "simplification" of the API, changing the position in the enhancer execution stack.

They state a few times, that if you are not using any different enhancers or middleware, you can just use the devToolsEnhancer. But when you are using other enhancers or middleware, you should be using the composeWithDevTools to wrap your other enhancers.

In this regard, I am taking a similar approach. I am adding a new method composeWithStateSync, which resembles the composeWithDevTools. Both methods solve the issue with the order by basically wrapping the compose and making sure the enhancers are in the correct position in the chain. You'll still be able to just use the stand-alone enhancer. But the same thing applies here. You'll need the custom compose function if you have other enhancers.

I actually just got it working as well.

@Nantris
Copy link
Contributor

Nantris commented Feb 9, 2021

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 replayActionRenderer will need to be reintroduced?

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)

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 9, 2021

@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,replayActionRenderer will not need to be introduced. Something extra will just be introduced to replace the current enhancer.

And no, I just removed those hacks while now preparing these changes, so I will not be able to share these anymore.

@matmalkowski
Copy link
Collaborator

@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 composeWithStateSync that would come as a bonus for more complex scenarios. It doesn't sound like a significant lib API change TBH

@Nantris
Copy link
Contributor

Nantris commented Feb 9, 2021

Whoops, it looks like it "works" for me because I'd forgotten I tried this lunacy of running applyMiddleware both before AND after the electron-redux enhancers are included.

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

@sneljo1
Copy link
Contributor Author

sneljo1 commented Feb 9, 2021

@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. 🙂

@Nantris
Copy link
Contributor

Nantris commented Feb 9, 2021

@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!

matmalkowski added a commit that referenced this issue Feb 16, 2021
…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]>
@matmalkowski
Copy link
Collaborator

🎉 This issue has been resolved in version 2.0.0-alpha.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

matmalkowski added a commit that referenced this issue Oct 2, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 released on @alpha v2 All issues related to v2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants