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

feat(storage-plugin): allow providing feature states #1994

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

arturovt
Copy link
Member

@arturovt arturovt commented Apr 11, 2023

Issue: #850

This commit introduces the ability to specify states for persistence at the feature level.
Previously, you could only specify a list of states for persistence when providing the storage
plugin at the root level. This feature is only available with the standalone API, as Angular has
an ENVIRONMENT_INITIALIZER feature that allows us to add these states at the feature level.

The API is simple and straightforward. It introduces the withStorageFeature function, which
can be called along with provideStates. It requires a list of states to be provided. If all
states are already being persisted because the developer specified keys: * at the root level,
this won't do anything.

@markwhitfeld
Copy link
Member

Very interesting work going on here! 🎉
Could you add a description to the PR (as well as a linked issue or discussion if applicable) so that the problem, and proposed solution are clear?

@arturovt arturovt changed the base branch from v-next to master October 2, 2023 16:09
@arturovt arturovt marked this pull request as ready for review October 2, 2023 16:23
@nx-cloud
Copy link

nx-cloud bot commented Oct 2, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4d8ac9a. 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 3 targets

Sent with 💌 from NxCloud.

@bundlemon
Copy link

bundlemon bot commented Oct 2, 2023

BundleMon (Integration Projects)

Files updated (2)
Status Path Size Limits
Main bundles(Gzip)
hello-world-ng16/dist-integration/main.(hash)
.js
66.85KB (+96B +0.14%) +1%
Main bundles(Gzip)
hello-world-ng17/dist-integration/main.(hash)
.js
68.38KB (+91B +0.13%) +1%

Total files change +187B +0.14%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really awesome feature!
We need a bit more information of what this feature looks like, so could you add a section in the docs about it, or describe it in the PR description, so that what the feature looks like is clear for review.
Is there a linked issue or discussion? This may assist in making the the problem, and proposed solution clearer.

@@ -12,7 +12,7 @@ export interface KeyWithExplicitEngine {

/** Determines whether the provided key has the following structure. */
export function ɵisKeyWithExplicitEngine(key: any): key is KeyWithExplicitEngine {
return key != null && !!key.engine;
return !!key?.engine;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although very succinct, it is difficult to understand this. Adding brackets would help with understanding.

Suggested change
return !!key?.engine;
return !!(key?.engine);

Caveat... if the prettier formatter wants to remove the brackets, then we would have to rather revert to the previous longer form of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return !!key?.engine;
return Boolean(key?.engine);

Comment on lines 65 to 67
NgxsStoragePluginModule.forRoot({
key: [CounterState]
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to test the case where the root plugin has been initialised with no keys present (therefore every state is stored), and then the blog route is loaded, which will set a key and engine. Will the existing states be stored? I'm not sure if the current logic handles this scenario.

I think the expectation would be that all states would be stored in the default key, and that explicitly specified feature states would be stored separately.
Maybe we need to discuss what would happen for any other feature states... What is the current behaviour for lazy feature states?


@Injectable({ providedIn: 'root' })
export class ɵNgxsStoragePluginKeysManager {
readonly keysWithEngines: KeyWithEngine[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing this breaks encapsulation. We need to discuss alternatives to exposing this property.

Copy link

nx-cloud bot commented Feb 16, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 046d028. 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.

@arturovt arturovt force-pushed the feat/storage-feature branch 6 times, most recently from dc7dfd0 to 589d87b Compare March 12, 2024 20:42
This commit introduces the ability to specify states for persistence at the feature level.
Previously, you could only specify a list of states for persistence when providing the storage
plugin at the root level. This feature is only available with the standalone API, as Angular has
an `ENVIRONMENT_INITIALIZER` feature that allows us to add these states at the feature level.

The API is simple and straightforward. It introduces the `withStorageFeature` function, which
can be called along with `provideStates`. It requires a list of states to be provided. If all
states are already being persisted because the developer specified `keys: *` at the root level,
this won't do anything.
Copy link

codeclimate bot commented Mar 13, 2024

Code Climate has analyzed commit 046d028 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

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

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

View more on Code Climate.

@arturovt arturovt merged commit 8085e85 into master Mar 13, 2024
12 of 13 checks passed
@arturovt arturovt deleted the feat/storage-feature branch March 13, 2024 01:26
@markwhitfeld markwhitfeld added this to the v.18.0.0 milestone Jun 24, 2024
@isochronous
Copy link
Contributor

Please, add some documentation around this withStorageFeature function

@isochronous
Copy link
Contributor

And does this method support the beforeSerialize and afterDeserialize lifecycle methods that are provided via the .forRoot version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants