-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add support for translations in actual addons #481
Conversation
bedf499
to
1531a58
Compare
1531a58
to
2ce9f10
Compare
2ce9f10
to
a1a139d
Compare
3304981
to
4f9e801
Compare
4f9e801
to
a307d87
Compare
a307d87
to
33cb043
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.
LGTM, i think the new opt in approach is best and it gives devs flexibility, Im still not a huge fan of using addon translations, but i think this works better then whitelisting the ones you are using as they can easily go out of date quickly. Thanks for your work and i'll look to create new release with your changes tomorrow
Thanks for the approval! I understand your thoughts about addon translations, so maybe good to quickly elaborate on how we use them. Our app consists of a few in-repo addons and a few actual addons, which are scoped dependencies, called So that's why we can safely rely on using some translations from these addons in our app, and it's the reason why I wanted to add support for it here :) While I'm at it, do you think this PR where I added support for the |
Similar to #480, but this PR adds support for actual addons, also issued in #5 (and closes #5). Not too sure if this covers all scenarios (I just looked up
translations
folders innode_modules/*
andnode_modules/@*/*
, the latter for scoped addons), but at least it works for our codebase.To elaborate a bit: translations in actual addons are available for use in an app, but since they're not owned by the app, they show up under
Missing translations
(the ones that are used). Just adding them to thetranslationFiles
won't work properly yet, as that would cause such translations to end up underUnused translations
while they could still be used by the addon itself (and that's not the purpose here). So, I split it up into own translations and addon translations, so that just the own translations can be used to detect unused translations, and the maps combined to detect missing translations.Note that I didn't add tests yet, I'd first like to have some confirmation that this a good approach.After I added a test for in-repo addons, it was quite simple to add a test for actual addons as well.