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: pipe only loads necessary dependencies #395

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaitlynekdahl
Copy link

When using multiple scopes, Transloco pipe will derive scope from the key and only load dependencies for that scope.

Closes #394

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Issue Number: #394

When using multi flag, Transloco flag will map over and load dependencies for all scopes in the ProviderScope array, even if those fetched translations aren't needed for the view currently rendered. This might result in performance issues due to unnecessary API calls.

For example, the following component from the sample would only need translations for the admin-page scope, but the network tab shows that an unneeded call was also made for lazy-page translations:

image

What is the new behavior?

The Transloco pipe should derive the scope in question from the key passed in, then determine which scope to resolve by searching the ProviderScope array for a matching scope or alias name.

After these changes, we can see that the unneeded call for lazy-page translations is no longer made:

image

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

When using multiple scopes, Transloco pipe will derive scope from the key and only load dependencies for that scope.

Closes jsverse#394
@shaharkazaz
Copy link
Collaborator

@kaitlynekdahl Thanks for the very detailed issue 🙂
What is the point of defining multiple scopes if they are not in use? the assumption is that the scopes defined are used by the components/sub-components and therefore we should load them.

@kaitlynekdahl
Copy link
Author

Hey @shaharkazaz, a specific scenario that I've seen is the need for all scopes to be imported at the root module level so that services that have providedIn: root will still be able to use Transloco and inline loaders. Because of these modules being imported at the root, any Transloco pipes at the root level will end up loading all the translations.

I think your assumption is valid in the case that an app is only using lazily-loaded modules without singleton services, though. Are you thinking the above scenario should be handled differently?

@shaharkazaz
Copy link
Collaborator

@kaitlynekdahl Hey! sorry for the late response.
Looking again at your example, what's the benefit of setting both scopes in the providers list if not both are needed?
The loading logic is "if it's provided, it's needed", maybe the solution here should be where you are declaring your scopes.

If I misunderstood something, please feel free to share a more detailed explanation regarding your app's needs!
A dedicated repo is always the best way 😄

@negue
Copy link

negue commented Jun 16, 2021

I'd like this feature as well.

Currently I have components which may use different scopes inside (in order to not duplicate these strings), But adding all these scopes for each component ( / module) just takes too much time (the more components / module you split up the more scopes you need to add on each module)

Thats why I also just list all scopes in the TranslocoRootModule - and then use the strings everywhere I want.

But that way it'll load all scopes on the first resolve which you might not need (yet)

This might be outside of the norm of Transloco but once you need to split up components into more modules (in order to create smaller bundles) the current way just adds more / repetitive work of adding/checking all scopes that are needed per component.

@shaharkazaz
Copy link
Collaborator

@kaitlynekdahl, this will cause a side effect. For example, if you have multiple components with different scopes that should be rendered simultaneously, there could be a delay between them depending on the latency.

If you are not worried about this side effect, we'll accept a modified PR where your code is optional.

@negue
Copy link

negue commented Sep 22, 2021

Any update on this? :)

@shaharkazaz
Copy link
Collaborator

@negue waiting for @kaitlynekdahl to address my comments 🙂

@kaitlynekdahl
Copy link
Author

@kaitlynekdahl, this will cause a side effect. For example, if you have multiple components with different scopes that should be rendered simultaneously, there could be a delay between them depending on the latency.

If you are not worried about this side effect, we'll accept a modified PR where your code is optional.

@shaharkazaz sorry for the even slower response! When you say there could be a delay between the components, are you saying there'd be a flash of untranslated content while the scopes are resolved synchronously?

Also for making the code optional, are you thinking there'd be a flag passed into the params map?

Example usage:

// in the template 
<span class="admin-page-title">{{ 'AdminPageAlias.title' | transloco: {singleScope: true} }}</span>

Then we can use the singleScope param to determine whether or not to use the existing logic or the proposed logic in this PR. Does that sound like an appropriate solution?

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

Successfully merging this pull request may close these issues.

(Multi-scope) Transloco pipe unnecessarily loads dependencies for all scopes in ProviderScope array
3 participants