feat(store): ofAction* methods should have strong types #1808
Conversation
bbarry
requested review from
arturovt,
Carniatto,
joaqcid,
kuncevic,
markwhitfeld,
poloagustin and
splincode
as code owners
|
@arturovt @markwhitfeld could you see please? |
|
It depends on what is considered a breaking change. This change does not introduce any new functionality or have any impact on runtime code at all. Yet once merged, users running the version post change may find their application fails to compile if they were using these functions in such a way that they already had a defect in their code. |
Well, that is what considered a breaking change if apps will fail to compile. |
There was a problem hiding this comment.
@bbarry I am a BIG fan of this change, and it aligns perfectly with everything else going into the next minor release. We are adding some massive typings improvements 🎉 .
Requested Change: Could you update the changelog in this PR with a description of this improvement?
Also, a question, is there a minimum version of Typescript that is required for these changes?
Regarding the 'breaking change', I think that even though there may be compile failures when users upgrade to this, it is identifying issues that exist in their code. I would class this as a 'fixing change' instead and a welcome improvement in any codebase.
I can't think of a single case where a user might be unhappy that the upgrade has showcased a bug.
@arturovt and @splincode, I would be happy for this to be included in the next minor release as a non "breaking change" if you agree with my logic here?
I think it's not problem! TypeScript does exactly the same in its minor versions |
I believe the minimum version of typescript for this change is 2.8 but I am unable to test and verify (2.8 is when |
When using the actions stream and the ofAction* operators, restrict the
parameters of these functions to valid action types and the resulting stream
to the union type of the set of actions supplied to the operators.
Eg, before:
```
this.store.actions.pipe(
ofActionSuccessful(MyAction1, MyAction2),
tap((a/* : any */) => { });
```
After:
```
this.store.actions.pipe(
ofActionSuccessful(MyAction1, MyAction2),
tap((a/* : MyAction1 | MyAction2 */) => { });
```
This change may identify broken code in downstream applications.
closes ngxs#1807
|
@markwhitfeld is that description better? |
|
@bbarry Thank you for your updates. Could you add an entry in @splincode Unfortunately Typescript does not follow strict semantic versioning because they introduce multiple breaking changes in each minor release, so they are not quite a goldens standard 😁 . But, thank you for your vote on this. I'll wait to hear from @arturovt too. Hopefully he is still ok with all the bombing going on in his city 😭 . |
|
PS. @bbarry If you are particularly skilled in TypeScript types magic, then please DM me on Twitter ( |
|
Hey, as I said I love the change and I don't mind considering it as a fix. |
|
Waoh, that the build on master is failing from merging this PR. I see now that the CI checks never ran on this PR. Very strange indeed. |
This reverts commit e58cb22. (This has been reverted as part of pushing a patch release out with no new features)
This reverts commit 1508fea. (This has been reverted as part of pushing a patch release out with no new features)
This reverts commit e58cb22. (This has been reverted as part of pushing a patch release out with no new features)
This reverts commit e58cb22. (This has been reverted as part of pushing a patch release out with no new features)
This reverts commit 095d21d.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@ngxs/form-plugin](https://github.com/ngxs/store) | dependencies | minor | [`3.7.6` -> `3.8.0`](https://renovatebot.com/diffs/npm/@ngxs%2fform-plugin/3.7.6/3.8.0) | | [@ngxs/storage-plugin](https://github.com/ngxs/store) | dependencies | minor | [`3.7.6` -> `3.8.0`](https://renovatebot.com/diffs/npm/@ngxs%2fstorage-plugin/3.7.6/3.8.0) | | [@ngxs/store](https://github.com/ngxs/store) | dependencies | minor | [`3.7.6` -> `3.8.0`](https://renovatebot.com/diffs/npm/@ngxs%2fstore/3.7.6/3.8.0) | --- ### Release Notes <details> <summary>ngxs/store</summary> ### [`v3.8.0`](https://github.com/ngxs/store/blob/HEAD/CHANGELOG.md#​380-2023-03-29) [Compare Source](ngxs/store@v3.7.6...v3.8.0) - Feature: Build packages in Ivy format [#​1945](ngxs/store#1945) - Feature: Add advanced selector utilities [#​1824](ngxs/store#1824) - Feature: Expose `ActionContext` and `ActionStatus` [#​1766](ngxs/store#1766) - Feature: `ofAction*` methods should have strong types [#​1808](ngxs/store#1808) - Feature: Improve contextual type inference for state operators [#​1806](ngxs/store#1806) [#​1947](ngxs/store#1947) - Feature: Enable warning on unhandled actions [#​1870](ngxs/store#1870) [#​1951](ngxs/store#1951) - Feature: Router Plugin - Provide more actions and navigation timing option [#​1932](ngxs/store#1932) - Feature: Storage Plugin - Allow providing namespace for keys [#​1841](ngxs/store#1841) - Feature: Storage Plugin - Enable providing storage engine individually [#​1935](ngxs/store#1935) - Feature: Devtools Plugin - Add new options to the `NgxsDevtoolsOptions` interface [#​1879](ngxs/store#1879) - Feature: Devtools Plugin - Add trace options to `NgxsDevtoolsOptions` [#​1968](ngxs/store#1968) - Feature: Form Plugin - Allow `ngxsFormDebounce` to be string [#​1972](ngxs/store#1972) - Performance: Tree-shake patch errors [#​1955](ngxs/store#1955) - Fix: Get descriptor explicitly when it's considered as a class property [#​1961](ngxs/store#1961) - Fix: Avoid delayed updates from state stream [#​1981](ngxs/store#1981) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yNC41IiwidXBkYXRlZEluVmVyIjoiMzUuMjQuNSJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1837 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Improving the type definitions of the ofAction pipe operators improves the type of the
rxjs pipe and subscriptions and helps identify bugs in user code.
closes #1807
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1807
What is the new behavior?
ofAction* functions now have strong type restrictions (no more
any)Does this PR introduce a breaking change?
This change may nominally break application code in that it will uncover existing defects in said code where the application is passing types to these functions which will never be matched, or is attempting to use the pipe downstream in a way that can never occur. Every fix will be case specific and involves correcting the action type definition and/or downstream pipe methods.
Other information
Thank you https://github.com/typescript-eslint/typescript-eslint for providing the rule https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unsafe-argument.md which made me identify a dozen such defects that the changes here also error on.