feat(store): improve selector decorator types (#1929) #2042

ConnorUllmann · 2023-08-12T22:29:40Z

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
[x] 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?

The @Selector decorator does not enforce that the arguments of the decorated function match the return types of the selectors that it depends on.

Issue Number: #1929

What is the new behavior?

The @Selector decorator enforces that the arguments of the functions that it decorates match the return values of the selectors that it was passed.

Limitations:

The models of state classes are not inferred. Any type is allowed for these arguments (however StateToken works correctly).
The arguments list is checked against both the possibility of an injected state and the possibility of no injected state. This is regardless of the actual setting, and regardless of whether the decorator is used in a State class or in an independent class.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Although this is not a breaking change in the sense that a codebase with correct typings on their selector functions will require no changes, the purpose of this change is to expose failures to do that typing correctly and prevent them in the future. As a result, I would expect any reasonably-sized codebase to require fixes to their selector implementations as a result of this change.

nx-cloud · 2023-08-12T22:33:08Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d6be8b6. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

@selector

Improves @selector decorator so that the return types of the selectors passed to the decorator must match the types of the arguments of the decorated function. Closes ngxs#1929

bundlemon · 2023-08-14T17:27:52Z

BundleMon

Unchanged files (3)

Status	Path	Size	Limits
✅	fesm2015/ngxs-store.js	94.13KB	125KB / +0.5%
✅	fesm2015/ngxs-store-operators.js	6.23KB	15KB / +0.5%
✅	fesm2015/ngxs-store-internals.js	3.57KB	20KB / +0.5%

Total files change -2B 0%

Groups updated (1)

Status	Path	Size	Limits
✅	@ngxs/store(esm2015)[gzip] _{./esm2015/*/.js}	180.3KB (+435B +0.24%)	+1%

Unchanged groups (2)

Status	Path	Size	Limits
✅	@ngxs/store(umd)[gzip] _{./bundles/*.umd.js}	36.44KB	+1%
✅	@ngxs/store(fesm2015)[gzip] _{./fesm2015/*.js}	24.94KB	+1%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

bundlemon · 2023-08-14T17:27:55Z

BundleMon (NGXS Plugins)

Unchanged files (16)

Status	Path	Size	Limits
✅	Plugins(umd)[gzip] _{storage-plugin/bundles/ngxs-storage-plugin.um d.js}	7.97KB	+1%
✅	Plugins(umd)[gzip] _{router-plugin/bundles/ngxs-router-plugin.umd. js}	7.19KB	+1%
✅	Plugins(umd)[gzip] _{hmr-plugin/bundles/ngxs-hmr-plugin.umd.js}	6.89KB	+1%
✅	Plugins(umd)[gzip] _{websocket-plugin/bundles/ngxs-websocket-plugi n.umd.js}	6.79KB	+1%
✅	Plugins(fesm2015)[gzip] _{storage-plugin/fesm2015/ngxs-storage-plugin.j s}	3.7KB	+1%
✅	Plugins(umd)[gzip] _{form-plugin/bundles/ngxs-form-plugin.umd.js}	3.29KB	+1%
✅	Plugins(fesm2015)[gzip] _{router-plugin/fesm2015/ngxs-router-plugin.js}	2.92KB	+1%
✅	Plugins(umd)[gzip] _{devtools-plugin/bundles/ngxs-devtools-plugin. umd.js}	2.75KB	+1%
✅	Plugins(fesm2015)[gzip] _{hmr-plugin/fesm2015/ngxs-hmr-plugin.js}	2.65KB	+1%
✅	Plugins(fesm2015)[gzip] _{form-plugin/fesm2015/ngxs-form-plugin.js}	2.57KB	+1%
✅	Plugins(umd)[gzip] _{logger-plugin/bundles/ngxs-logger-plugin.umd. js}	2.53KB	+1%
✅	Plugins(fesm2015)[gzip] _{websocket-plugin/fesm2015/ngxs-websocket-plug in.js}	2.52KB	+1%
✅	Plugins(fesm2015)[gzip] _{devtools-plugin/fesm2015/ngxs-devtools-plugin .js}	2.17KB	+1%
✅	Plugins(fesm2015)[gzip] _{logger-plugin/fesm2015/ngxs-logger-plugin.js}	2.01KB	+1%
✅	Plugins(umd)[gzip] _{router-plugin/bundles/ngxs-router-plugin-inte rnals.umd.js}	654B	+1%
✅	Plugins(fesm2015)[gzip] _{router-plugin/fesm2015/ngxs-router-plugin-int ernals.js}	406B	+1%

No change in files bundle size

Unchanged groups (3)

Status	Path	Size	Limits
✅	All Plugins(esm2015)[gzip] _{./-plugin/esm2015/**/.js}	109.06KB	+1%
✅	All Plugins(umd)[gzip] _{./-plugin/bundles/.umd.js}	38.05KB	+1%
✅	All Plugins(fesm2015)[gzip] _{./-plugin/fesm2015/.js}	18.93KB	+1%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

bundlemon · 2023-08-14T17:27:58Z

BundleMon (Integration Projects)

Unchanged files (2)

Status	Path	Size	Limits
✅	Main bundles(Gzip) _{hello-world-ng17/dist-integration/main.(hash) .js}	68.5KB	+1%
✅	Main bundles(Gzip) _{hello-world-ng16/dist-integration/main.(hash) .js}	67.03KB	+1%

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

nx-cloud · 2024-04-05T13:23:34Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a4eaf0b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

markwhitfeld

@ConnorUllmann Thank you so much for this contribution!
My apologies for the long delay in reviewing this.
We would like to include your PR in NGXS v17, which is landing very soon.

Would it be possible to add some comments / descriptions to the types that you created which explain how they work / what they do / etc. so that it is easier for us to be able to understand and maintain this code into the future?

One other quick comment is that after this PR is merged, we would like to build on top of your changes to add a deprecation notice to any occurrence of the @Selector(...) decorator where the selector function is expecting the container state as the first parameter (except for the parameterless version). I am just trying to figure out where exactly that doc comment would be 😄 .

ConnorUllmann · 2024-04-05T20:09:36Z

@markwhitfeld Glad to hear it! I'm writing up the descriptions of those types now.

I had to make some modifications in order to allow the parameterless call to not be deprecated when injecting container state while deprecating the version of the call with parameters. Do the examples in the image below accurately cover what you're expecting to see deprecated?

I can include the deprecation myself in my update, if you like, otherwise I'll note where it will need to be put.

markwhitfeld · 2024-04-05T20:46:31Z

Yes, I agree with what you have there.
If you would like to add the deprecation notice, it would be the same as the one found here:

store/packages/store/internals/src/symbols.ts

Line 86 in 3bbb9e1

    
              * Read the deprecation notice at this link: https://ngxs.io/deprecations/inject-container-state-deprecation.md.

The deprecation notice should point to the page that will go live when the release goes out:
https://ngxs.io/deprecations/inject-container-state-deprecation.md
You can see the development version of this page here.

…ntainer state call signature

markwhitfeld

Fantastic work.
Can I ask you to name the T and U type placeholders according to what they are?
This should be the last puzzle piece to make this more understandable and therefore maintainable.

codeclimate · 2024-04-06T14:25:01Z

Code Climate has analyzed commit a4eaf0b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 95.3% (0.0% change).

View more on Code Climate.

markwhitfeld

Thank you so much. This is really super work!

ConnorUllmann force-pushed the typed-selector-decorator branch 2 times, most recently from e468148 to 71e1ad5 Compare August 13, 2023 06:46

ConnorUllmann marked this pull request as ready for review August 13, 2023 07:25

ConnorUllmann requested review from markwhitfeld, arturovt, kuncevic, Carniatto, poloagustin, joaqcid and splincode as code owners August 13, 2023 07:25

feat(store): improve selector decorator types (ngxs#1929)

d6be8b6

Improves @selector decorator so that the return types of the selectors passed to the decorator must match the types of the arguments of the decorated function. Closes ngxs#1929

ConnorUllmann force-pushed the typed-selector-decorator branch from 71e1ad5 to d6be8b6 Compare August 13, 2023 16:27

Merge branch 'master' into typed-selector-decorator

5879841

fix: mistake in merge resolution

506803e

markwhitfeld reviewed Apr 5, 2024

View reviewed changes

Merge branch 'ngxs:master' into typed-selector-decorator

0b4169c

ngxs deleted a comment from codeclimate bot Apr 5, 2024

ConnorUllmann added 2 commits April 5, 2024 17:06

feat(store): add selector type descriptions and deprecate injected co…

2d2e81a

…ntainer state call signature

Merge branch 'master' into typed-selector-decorator

b118e31

markwhitfeld requested changes Apr 6, 2024

View reviewed changes

feat(store): rename selector decorator type parameters

a4eaf0b

arturovt approved these changes Apr 6, 2024

View reviewed changes

ConnorUllmann requested a review from markwhitfeld April 6, 2024 14:42

markwhitfeld approved these changes Apr 6, 2024

View reviewed changes

markwhitfeld merged commit f52c613 into ngxs:master Apr 6, 2024
12 checks passed

ConnorUllmann deleted the typed-selector-decorator branch April 7, 2024 23:07

markwhitfeld added this to the v.18.0.0 milestone Jun 24, 2024

markwhitfeld added the released label Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): improve selector decorator types (#1929) #2042

feat(store): improve selector decorator types (#1929) #2042

ConnorUllmann commented Aug 12, 2023

nx-cloud bot commented Aug 12, 2023 •

edited

Loading

bundlemon bot commented Aug 14, 2023

bundlemon bot commented Aug 14, 2023

bundlemon bot commented Aug 14, 2023 •

edited

Loading

nx-cloud bot commented Apr 5, 2024 •

edited

Loading

markwhitfeld left a comment •

edited

Loading

ConnorUllmann commented Apr 5, 2024 •

edited

Loading

markwhitfeld commented Apr 5, 2024

markwhitfeld left a comment

codeclimate bot commented Apr 6, 2024

markwhitfeld left a comment

feat(store): improve selector decorator types (#1929) #2042

feat(store): improve selector decorator types (#1929) #2042

Conversation

ConnorUllmann commented Aug 12, 2023

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

nx-cloud bot commented Aug 12, 2023 • edited Loading

☁️ Nx Cloud Report

bundlemon bot commented Aug 14, 2023

BundleMon

bundlemon bot commented Aug 14, 2023

BundleMon (NGXS Plugins)

bundlemon bot commented Aug 14, 2023 • edited Loading

BundleMon (Integration Projects)

nx-cloud bot commented Apr 5, 2024 • edited Loading

☁️ Nx Cloud Report

markwhitfeld left a comment • edited Loading

Choose a reason for hiding this comment

ConnorUllmann commented Apr 5, 2024 • edited Loading

markwhitfeld commented Apr 5, 2024

markwhitfeld left a comment

Choose a reason for hiding this comment

codeclimate bot commented Apr 6, 2024

markwhitfeld left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Aug 12, 2023 •

edited

Loading

bundlemon bot commented Aug 14, 2023 •

edited

Loading

nx-cloud bot commented Apr 5, 2024 •

edited

Loading

markwhitfeld left a comment •

edited

Loading

ConnorUllmann commented Apr 5, 2024 •

edited

Loading