-
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
feat(store): expose StateContextFactory, StateFactory #1325
Conversation
For the plugin `@ngxs-labs/data` needs a more api from `@ngxs/store`. Because `@ngxs-labs/data` use deep internal manipulation with Store.
@markwhitfeld Please, I need this functionality in production right now. Therefore, I want to use this functionality in our projects. Now I can’t wait, so on production I will use {
"@ngxs/store": "3.5.1@dev",
"@ngxs-labs/data": "1.0.1"
} This functionality is really necessary for the plugin. |
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.
@splincode As mentioned on the reported issue (#1323) I really do not want to expose the internals of NGXS for plugins. This can compromise the fundamentals of the framework and cause instability in the plugins if we change these internals at a later stage.
The only option that I would accept here is to expose these from the @ngxs/store/internals
subpackage. The way I can see to achieve this would be to expose an InjectionToken that will be used for each of these from the internals subpackage. Then those InjectionTokens would be used to provide those classes to the injector within the main @ngxs/store
package. In this way these classes will remain in the main package but are accessible to anyone that wants to risk referencing something from the @ngxs/store/internals
subpackage. You could expose TS type
definitions for these from the internals subpackage too if you like, but not the underlying classes (these should remain in the main package).
(remember that the internals subpackage can receive breaking changes without affecting the semantic versioning)
This the best that I can offer.
@markwhitfeld updated |
@markwhitfeld done |
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.
Great!
PS. I'm not sure what the conventions are for naming tokens. I assumed that the _TOKEN
suffix is the convention based on what already existed in the file.
@arturovt could you review again?
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 pre-approving that.
So after our comments you're free to merge this 😉
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.
Great!
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.
Sure, I’m ok with that. |
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.
Guess we have to drop the support for ng5 as it's like to support mammoths 😁
@arturovt only in V4 |
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: #1323
What is the new behavior?
For the plugin
@ngxs-labs/data
needs a more api from@ngxs/store
. Because@ngxs-labs/data
use deep internal manipulation with Store.Does this PR introduce a breaking change?