fix(store): do not infer T from argument to patch #1806

david-shortman · 2021-11-26T15:47:02Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngxs/store/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

david-shortman · 2021-11-26T15:49:14Z

@markwhitfeld Some questions in order to author a full PR-

What is the scope of what should be fixed? All operators in the operators directory?
Should a newer type testing library like ts-snippet be used to assert the functionality of the updates?
Should NoInfer be exposed for consumers? Or should this PR publish another mechanism (like a function, if possible) for devs to create their own operators?

packages/store/operators/src/patch.ts

markwhitfeld · 2021-11-27T19:51:14Z

@david-shortman Thank you for this PR! Looking good. Please ping me when you take it out of draft.
I was playing with this trick in a TypeScript playground and it almost brought a tear to my eye. The experience of using state operators is incredible again! Even better than when I first created them. I am so ecstatic!!!

Answers to your questions:

I think it would be great to fix the operators that may suffer from the parameter interfering with the inference... which I suspect will be most, if not all, of them. You can do them all in this PR, or you can split them up... up to you!
I think we stick to DTSLint in this PR, and then introduce a different approach in another PR, so that we can demonstrate the tradeoffs and potentially the scenarios that are better covered with another tool. PS. I have no problem with using both tools for different reasons if there is merit in that.
I think that it makes sense for NoInfer to be exposed publically for consumers implementing their own operators. Potentially we could include some guidance docs in a later PR for creating your own custom operators.

PS. I see that that NoInfer can be applied in different ways with the same effect:
NoInfer<PatchSpec<T>> or PatchSpec<NoInfer<T>>
I have no real opinion on which is better or more obvious in terms of what it does. What do you think?

david-shortman · 2021-12-01T03:04:02Z

@david-shortman Thank you for this PR! Looking good. Please ping me when you take it out of draft. I was playing with this trick in a TypeScript playground and it almost brought a tear to my eye. The experience of using state operators is incredible again! Even better than when I first created them. I am so ecstatic!!!

Answers to your questions:

I think it would be great to fix the operators that may suffer from the parameter interfering with the inference... which I suspect will be most, if not all, of them. You can do them all in this PR, or you can split them up... up to you!

I think we stick to DTSLint in this PR, and then introduce a different approach in another PR, so that we can demonstrate the tradeoffs and potentially the scenarios that are better covered with another tool. PS. I have no problem with using both tools for different reasons if there is merit in that.

I think that it makes sense for NoInfer to be exposed publically for consumers implementing their own operators. Potentially we could include some guidance docs in a later PR for creating your own custom operators.

PS. I see that that NoInfer can be applied in different ways with the same effect: NoInfer<PatchSpec<T>> or PatchSpec<NoInfer<T>> I have no real opinion on which is better or more obvious in terms of what it does. What do you think?

I'll try to update all operators as needed.
👍
Will export NoInfer without any docs for now
PS. Great point. Not only does PatchSpec<NoInfer<T>> read more logically (we want to tell TS to not infer T, not some complex type built using T), but TS infers the type of the argument correctly that way such that I don't have to cast it as I claimed I "needed" to in this comment.

markwhitfeld · 2021-12-02T08:02:19Z

PS, regarding 2, can TS snippet check the type inferred (and therefore the intellisense auto-completion) for an argument at a call site. I would be amazed if any tool could test this crazy requirement!

markwhitfeld · 2021-12-02T08:04:32Z

PPS. Amazing article by the way!
Leaving here for reference:
https://dev.to/davidshortman/weird-ts-types-using-contextual-typing-and-deferred-inference-to-plan-an-alien-conquest-bm8
PPSPS. Could you DM me on Twitter so that I can message you. Your DMs are currently closed.

david-shortman · 2021-12-03T02:30:44Z

packages/store/operators/src/compose.ts


-export function compose<T>(...operators: StateOperator<T>[]): StateOperator<T> {
+export function compose<T>(...operators: NoInfer<StateOperator<T>[]>): StateOperator<T> {


sometimes, Typescript was happier about the inferred internals of the function when wrapping NoInfer around the entire parameter type. other times, it was happier only wrapping T. so I have implemented a mixture with the principle being whatever results in the smallest code change for correct inference.

david-shortman · 2021-12-03T02:48:00Z

packages/store/operators/src/update-item.ts

+    const theOperatorOrValue = operatorOrValue as T | StateOperator<T>;
+    if (isStateOperator(theOperatorOrValue)) {
+      value = theOperatorOrValue(existing[index] as Readonly<T>);


Typescript will not narrow the type correctly, so I had to assert the type of operatorOrValue first.

david-shortman · 2021-12-03T03:10:11Z

@markwhitfeld The build is outputting an error that is not present in my IDE. When I put the relevant code into a TS playground using the same TS version as the repo, 3.9.7, there is also no error: playground.

Build error:

../../packages/store/operators/src/insert-item.ts(21,15): error TS2352: Conversion of type 'NoInfer' to type 'RepairType' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
../../packages/store/operators/src/insert-item.ts(35,28): error TS2352: Conversion of type 'NoInfer' to type 'RepairType' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

This same problem (build outputs error that is not reproducible in playground) occurs several times in the build.

markwhitfeld · 2021-12-03T22:47:16Z

@david-shortman Thank you for your thorough updates to this PR.
I will see if I can reproduce those build errors my side.

markwhitfeld · 2022-01-02T12:33:54Z

@david-shortman I have fixed the build issues, but this lead to discovering another crazy typings challenge with the iif operator. I have solved the iif issue (after a month of thrashing on this), but that would need to be resolved in a separate PR.
I have pushed to your branch (surprised that I was able to do so) and, hopefully, I can get this merged.

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

(This has been reverted as part of pushing a patch release out with no new features)

This reverts commit a43e17f. The effectively reinstates the changelog related to #1806 by reapplying the changes from commit 89968a0.

)"" This reverts commit eb56df0. The effectively reinstates the changes related to #1808 by reapplying the changes from commit 8834f50.

This reverts commit a43e17f. The effectively reinstates the changelog related to #1806 by reapplying the changes from commit 89968a0.

)"" This reverts commit eb56df0. The effectively reinstates the changes related to #1808 by reapplying the changes from commit 8834f50.

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

(This has been reverted as part of pushing a patch release out with no new features)

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

)"" This reverts commit 34f3973.

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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  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>

david-shortman commented Nov 26, 2021

View reviewed changes

packages/store/operators/src/patch.ts Outdated Show resolved Hide resolved

david-shortman commented Dec 3, 2021

View reviewed changes

david-shortman force-pushed the no-infer-operator-argument branch 3 times, most recently from df58d31 to 03deb07 Compare December 3, 2021 02:46

fix(store): do not infer T from argument to patch

7776099

david-shortman force-pushed the no-infer-operator-argument branch from 03deb07 to 7776099 Compare December 3, 2021 02:46

david-shortman commented Dec 3, 2021

View reviewed changes

fix(store): type casting failures

3157931

test(store): fix small typing issue

9646a40

markwhitfeld marked this pull request as ready for review January 2, 2022 13:01

markwhitfeld requested review from arturovt, Carniatto, joaqcid, kuncevic, markwhitfeld, poloagustin and splincode as code owners January 2, 2022 13:01

markwhitfeld approved these changes Jan 2, 2022

View reviewed changes

markwhitfeld merged commit 8834f50 into ngxs:master Jan 2, 2022

david-shortman deleted the no-infer-operator-argument branch January 16, 2022 16:21

markwhitfeld added a commit that referenced this pull request Jun 8, 2022

Revert "fix(store): do not infer T from argument to patch (#1806)"

eb56df0

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

markwhitfeld added a commit that referenced this pull request Jun 8, 2022

chore: revert changelog related to #1806

a43e17f

(This has been reverted as part of pushing a patch release out with no new features)

markwhitfeld added a commit that referenced this pull request Jun 10, 2022

Revert "chore: revert changelog related to #1806"

8c9423a

This reverts commit a43e17f. The effectively reinstates the changelog related to #1806 by reapplying the changes from commit 89968a0.

markwhitfeld added a commit that referenced this pull request Jun 10, 2022

Revert "chore: revert changelog related to #1806"

075490b

This reverts commit a43e17f. The effectively reinstates the changelog related to #1806 by reapplying the changes from commit 89968a0.

markwhitfeld added this to the v3.next-minor milestone Jun 11, 2022

markwhitfeld added a commit that referenced this pull request Aug 8, 2022

Revert "fix(store): do not infer T from argument to patch (#1806)"

95ab781

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

markwhitfeld added a commit that referenced this pull request Aug 8, 2022

chore: revert changelog related to #1806

48731a1

(This has been reverted as part of pushing a patch release out with no new features)

markwhitfeld added a commit that referenced this pull request Aug 8, 2022

Revert "fix(store): do not infer T from argument to patch (#1806)"

34f3973

This reverts commit 8834f50. (This has been reverted as part of pushing a patch release out with no new features)

markwhitfeld added a commit that referenced this pull request Mar 29, 2023

Revert "Revert "fix(store): do not infer T from argument to patch (#1806

0397eab

)"" This reverts commit 34f3973.

markwhitfeld modified the milestones: v3.next-minor, v3.8.0 Mar 29, 2023

markwhitfeld added the released label Mar 29, 2023

abdulrahman305 mentioned this pull request Jul 3, 2024

[Snyk] Upgrade: , , , , abdulrahman305/auptimizer#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(store): do not infer T from argument to patch #1806

fix(store): do not infer T from argument to patch #1806

david-shortman commented Nov 26, 2021

david-shortman commented Nov 26, 2021

markwhitfeld commented Nov 27, 2021

david-shortman commented Dec 1, 2021 •

edited

Loading

markwhitfeld commented Dec 2, 2021

markwhitfeld commented Dec 2, 2021

david-shortman Dec 3, 2021

david-shortman Dec 3, 2021

david-shortman commented Dec 3, 2021 •

edited

Loading

markwhitfeld commented Dec 3, 2021

markwhitfeld commented Jan 2, 2022 •

edited

Loading


		export function compose<T>(...operators: StateOperator<T>[]): StateOperator<T> {
		export function compose<T>(...operators: NoInfer<StateOperator<T>[]>): StateOperator<T> {

fix(store): do not infer T from argument to patch #1806

fix(store): do not infer T from argument to patch #1806

Conversation

david-shortman commented Nov 26, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

david-shortman commented Nov 26, 2021

markwhitfeld commented Nov 27, 2021

david-shortman commented Dec 1, 2021 • edited Loading

markwhitfeld commented Dec 2, 2021

markwhitfeld commented Dec 2, 2021

david-shortman Dec 3, 2021

Choose a reason for hiding this comment

david-shortman Dec 3, 2021

Choose a reason for hiding this comment

david-shortman commented Dec 3, 2021 • edited Loading

markwhitfeld commented Dec 3, 2021

markwhitfeld commented Jan 2, 2022 • edited Loading

david-shortman commented Dec 1, 2021 •

edited

Loading

david-shortman commented Dec 3, 2021 •

edited

Loading

markwhitfeld commented Jan 2, 2022 •

edited

Loading