-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(effects): accept ObservableInput as concatLatestFrom argument #3838
feat(effects): accept ObservableInput as concatLatestFrom argument #3838
Conversation
… observable + removes identity use of `pipe(fn)`. + removes use of `of(x).pipe(withLatestFrom(...args)))` in favor of just `forkJoin(args).pipe(map(r => [x, ...r]))` because it's more efficient and does basically the same thing. + No longer checks to see if there's an array + Changes the typings to use `ObservableInput` rather than `Observable`. The code always supported Promise, Iterable, AsyncIterable, etc, but the TypeScript typings prevented it.
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
Really my problem is I'm lazy and I'm using the GitHub in-browser IDE.
+ resolve an issue where `map` wasn't imported.
I'm not super happy with this solution, as it seems like this could all be more efficient. But it's terse and it works.
Looking at this, I just reverted to remove the superfluous Sorry for the churn BTW, the GitHub Web API isn't amazing for iterating on development, and I didn't feel like setting things up locally. Basically, I'd really need to understand the use-cases here, because it seems expensive to have to subscribe to N observables for each dispatched action. What I suspect is that most of the use cases this is serving would be better served by a more specialized set of code, but I don't have context. For example though, if I were to see that most of the use cases for this were selecting a current value from state, I'd say that it would be better to synchronously just pull that value from state with a simple function call rather than subscribe to a push... Which is the difference between In any case, that additional |
There's context related to why we introduced On the development side of things, we also have built-in support for using GitHub codespaces in this repo, so you can still edit on the web with a better interface. https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md |
pipe(fn)
.of(x).pipe(withLatestFrom(...args)))
in favor of justforkJoin(args).pipe(map(r => [x, ...r]))
because it's more efficient and does basically the same thing.ObservableInput
rather thanObservable
. The code always supported Promise, Iterable, AsyncIterable, etc, but the TypeScript typings prevented it.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Other information
This is a refactor with the exception of type changes. Sort of a drive-by on my part because someone pointed me to this operator during an interview. If y'all want to add type changes or have someone take this PR over and run with it, that's fine.
I made this PR with the GitHub online IDE. I have no idea if CI will pass.