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

Add support for translations in in-repo addons #480

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

robinborst95
Copy link
Contributor

@robinborst95 robinborst95 commented Mar 16, 2022

In our codebase I tried out this addon and I realized that in-repo addons were not supported yet (already issued a while ago). It seemed easy to add though, so I gave it a try to get it working for us at least. I got it to work, but before I look into adding tests, let me know if this is a good approach and if maybe more use cases with in-repo addons should be supported.

Note: I don't know if running the tests needs to be explicitly allowed because this is my first PR here, but locally the tests still pass at least. I added a test already as I was curious how the tests worked, so now there is proof it works :)

@robinborst95 robinborst95 marked this pull request as ready for review March 16, 2022 16:01
@robinborst95 robinborst95 force-pushed the feature/in-repo-paths branch 3 times, most recently from ec3d62b to 2bdc648 Compare March 25, 2022 14:17
@Mikek2252
Copy link
Collaborator

The changes look fine to me, but i'm not sure if the test case is correctly testing your changes. I think the test case should correctly catch whether there is unused translations with in the in-repo addon and also checking that an addon translations that is used in the app is not considered unused.

Also a question, would it better to run ember-intl-analyzer independently in that inrepo addon unless you know you will exporting those translations from the addon but not using them in the addon.

@Mikek2252 Mikek2252 added the enhancement New feature or request label Mar 25, 2022
@robinborst95
Copy link
Contributor Author

robinborst95 commented Mar 25, 2022

@Mikek2252 I'm not sure if I fully understand what you're saying, but maybe it's good that I explain my thoughts behind it. In our codebase we mainly use in-repo addons for the separation of concerns, so as if it's normal app code, but then located in lib. So, files in these addons use translations (and other things like components) that are in app and vice versa (although limited). For this reason we treat unused and missing translation in app in the same way as the in-repo addons, and that's also what I assumed for this PR, so adding the app and in-repo addon files together to look for missing and unused translations.

For the test case I didn't mix it indeed though, so in app I didn't use translations from the in-repo addon, and vice versa, so the tests wouldn't catch this when finding the in-repo addons would fail in the future, so I'll add that then 👍 (I think that's what you meant). Edit: I added it to the tests, as you can see here.

To answer your question about running it independently in the in-repo addon: I understand the question, but I think that is not necessarily desired (, at least not in our codebase). As in-repo addons are only used in the app that they're in, it makes sense that they can depend on translations that are not in the addon, so then it makes less sense to mark such translations as missing. If desired (and if support for addon next to app is added), you can always still run it independently in the addon of course, that just behaves as if you're running it in the app itself.

@robinborst95 robinborst95 force-pushed the feature/in-repo-paths branch 3 times, most recently from 55f5003 to e0163db Compare March 25, 2022 15:41
@Mikek2252
Copy link
Collaborator

@robinborst95 Yeah that seems reasonable enough was more curiosity. As for the test cases i was thinking you may want one that checks the command fails when :

  • a translation that is defined in the in-repo addon but is unused
  • that a translations that is used in the inrepo addon but is missing.

@robinborst95 robinborst95 force-pushed the feature/in-repo-paths branch from e0163db to 3511aea Compare March 25, 2022 19:51
@robinborst95
Copy link
Contributor Author

@Mikek2252 good suggestions, I've added them 👍

Copy link
Collaborator

@Mikek2252 Mikek2252 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry i have been busy this week, but LGTM. I will check out your other PR's before i create a release if i have any comments i will make a release so you dont need to wait to use the changes

@Mikek2252
Copy link
Collaborator

So it looks like an update to eslint 8 got merged which is why the node10 tests are failing, I will create a breaking release to officially drop support for node 10 then i will merge this

@robinborst95 robinborst95 force-pushed the feature/in-repo-paths branch from 8f37e32 to f59f331 Compare April 16, 2022 17:31
@Mikek2252 Mikek2252 merged commit 7b61f00 into mainmatter:master Apr 20, 2022
@robinborst95 robinborst95 deleted the feature/in-repo-paths branch April 22, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants