-
Notifications
You must be signed in to change notification settings - Fork 403
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
refactor(store): tree-shake internal state tokens #2246
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 0a93595. 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 targetsSent with 💌 from NxCloud. |
commit: |
BundleMonFiles updated (2)
Unchanged files (4)
Total files change +400B +0.3% Groups updated (2)
Final result: ✅ View report in BundleMon website ➡️ |
BundleMon (NGXS Plugins)Unchanged files (9)
No change in files bundle size Unchanged groups (1)
Final result: ✅ View report in BundleMon website ➡️ |
BundleMon (Integration Projects)Files updated (3)
Total files change -111B -0.05% Final result: ✅ View report in BundleMon website ➡️ |
066a020
to
35a8968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand the motivation here?
What is the practical benefit of the tree shaking? What problem are we solving?
Tokens are removed from the bundle. |
In this commit, we remove internal injection token providers from the root providers list. Instead, we expose a function that provides these tokens.
35a8968
to
0a93595
Compare
Code Climate has analyzed commit 0a93595 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 90.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. |
Merging because I want to proceed with the next patch release. This change only reduces the bundle size by removing tokens that are not used in standard apps. These tokens were previously exposed as part of the ngxs-labs/data package. |
import { | ||
ɵStateClass, | ||
ɵNGXS_STATE_CONTEXT_FACTORY, | ||
ɵNGXS_STATE_FACTORY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arturovt ɵNGXS_STATE_FACTORY
is used by @angular-ru/ngxs
☹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will need to use the ɵprovideNgxsInternalStateTokens
in their providers list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you inject the factory without having access to the injection token? State factory itself is used in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still inject the factory through the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Through which token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the phrase "having access to the token". Why don't you have access to the token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ngxs no longer exports ɵNGXS_STATE_FACTORY
and ɵNGXS_STATE_CONTEXT_FACTORY
tokens after this PR was merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't export it before either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, my bad. For some reason instead of import
, I thought export
was removed. The tokens are still exported from @ngxs/store/internals
. I'll try to use ɵprovideNgxsInternalStateTokens()
to see if it fixes the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ɵprovideNgxsInternalStateTokens()
fixes the issue. Sorry for the confusion.
In this commit, we remove internal injection token providers from the root providers list. Instead, we expose a function that provides these tokens.