-
-
Notifications
You must be signed in to change notification settings - Fork 592
feat(store): loaders #1596
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): loaders #1596
Conversation
if (this.loaders.has(loaderIndex)) this.loaders.delete(loaderIndex); | ||
} | ||
|
||
getLoaders() { |
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.
getLoaders() should deliver an array of unique loaders.
we should hide the information that we use the indices. it's a implementation detail that does not need to be shared with the service.
using this:
provideTranslateService({
loader: provideTranslateLoader(CustomLoader),
compiler: provideTranslateCompiler(CustomCompiler)
})
provideChildTranslateService({
// no loader
})
could add the same loader twice. this would lead to concurrent http requests for the same url.
this is why the list has to be unique.
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.
@CodeAndWeb I do not understand the following can you explain ?
we should hide the information that we use the indices
If you're talking about _loaderIndex
, I though the service should be aware of it, as he's removing "himself" from the store on destroy
- Same loader added twice.
You're right, but I may be missing expertise here, do you now how to handle this possibility ?
I though about generating a random id
per loader so I could check this loader, but it seems to be wrong.
But yet, we cannot get the instance ID in angular...
Doing so will force every use to add this value to theirs loaders.
But I do now understand why you wanted a counter, as it could be casted multiple time, you'd like to add, remove and delete it when it gets to 0.
Am I right ?
@@ -185,6 +185,8 @@ export class TranslateService implements ITranslateService { | |||
private missingTranslationHandler = inject(MissingTranslationHandler); |
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.
currentLoader is not used anymore. remove it.
it would only confuse users.
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.
Yes, I'll do once I'm doing the testing
... and finally, we need some tests for this functionality. |
Yes, I'll do the testing once we're done with the changes ;) |
@CodeAndWeb Thx for the help 😋 |
@CodeAndWeb Can you notify me once an RC is released, that I can test it out on some projects? Cheers |
Description
As we're now having multiple instance of the
TranslateService
, the only way to sync language across all services is through thestore
.As this issue describe, when adding a new loader, and changing the lang, the parent loader isn't triggered anymore.
Cause
The loading happen inside the service, but this service is not aware of the other loaders
Proposed solution
Storing each
loader
inside thestore
, and calling it when needed.loaders
map inside thestore
classloaders
mapFood for though
This PR is more "food for though" than a finite PR, as I'm aware following have not been solved