feat: 🧪 Support folder namespaces for intl #389

achambers · 2022-06-09T14:48:47Z

Fixes #344

Ember Intl exposes a configuration file which allows you to configure the localization setup. One such setting is wrapTranslationsWithNamespace which determines whether translation keys should have the folder names within the key names.

`wrapTranslationsWithNamespace: false` (default)

# translations/sub-folder/en-us.yml
subFolder:
  foo: bar

this.intl.t('subFolder.foo');

`wrapTranslationsWithNamespace: true`

# translations/sub-folder/en-us.yml
subFolder:
  foo: bar

this.intl.t('sub-folder.subFolder.foo');

wrapTranslationsWithNamespace: true is not currently supported by this extension. This PR adds the ability, when wrapTranslationsWithNamespace: true is set in app/config/ember-intl.js, to support keys which have folders in their key names.

codecov · 2022-06-09T17:00:07Z

Codecov Report

Merging #389 (d1e2554) into component-context-info-origin (4618d27) will increase coverage by 0.09%.
The diff coverage is 96.42%.

@@                        Coverage Diff                        @@
##           component-context-info-origin     #389      +/-   ##
=================================================================
+ Coverage                          72.63%   72.72%   +0.09%     
=================================================================
  Files                                 57       57              
  Lines                               4915     4935      +20     
  Branches                             959      966       +7     
=================================================================
+ Hits                                3570     3589      +19     
- Misses                              1345     1346       +1

Impacted Files	Coverage Δ
src/builtin-addons/core/intl-utils.ts	`95.90% <96.42%> (-0.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2def00...d1e2554. Read the comment docs.

lifeart · 2022-07-14T13:20:50Z

Hi @achambers! Do you need any help with PR?

patocallaghan · 2022-07-15T12:55:14Z

@lifeart 👋 I'm taking over this PR from @achambers as he is on vacation atm and we're both on the same team 😃 I've just pushed an update there which tries to follow some of your advice from #344 (comment). Could you have a look and see if I'm on the right track?

With regards testing this out I'm having a bit of difficulty. I'm struggling to manually verify my changes are working because I can't seem to make my VSCode see my local version of ELS rather than my installed one?

Also with regards actual tests writing unit tests might be tricky due to the reliance on needing the ember-intl.js config file. I added some fixtures which may be a better way to verify this change but I've yet to write a test yet. thoughts?

lifeart · 2022-07-15T14:10:35Z

Hey @patocallaghan! Direction is good!

According to tests, I think having few related unit tests should be fine:
Samples:

Also with regards actual tests writing unit tests might be tricky due to the reliance on needing the ember-intl.js config file. - we can use virtual fs provider (from Server class) to mock it with our own, without having it in files

here you could specify project file structure as 3rd argument (including intl config) - https://github.com/lifeart/ember-language-server/blob/component-context-info-origin/test/bultin-addons/core/intl-providers-test.ts#L73 to verify results

it's key-value structure where key is a path to file, and value is file content:

 await getResult(
              CompletionRequest.method,
              connection,
              {
               'app/components/test.hbs': '',
               'config/ember-intl.js': `module.exports = () => {  ... }`
              },
              'app/components/test.hbs',
              { line: 0, character: 0 }
            )
          ).response

patocallaghan · 2022-07-15T14:35:03Z

Thanks @lifeart, appreciate the advice. All makes sense. I'll be picking this up again on Monday 🙇‍♂️

patocallaghan · 2022-07-20T16:00:33Z

src/builtin-addons/core/intl-utils.ts

+      return null;
+    }
+
+    return typeof intlConfig === 'function' ? intlConfig() : intlConfig;


@lifeart This is perplexing me a bit as to how I should treat this intlConfig. Essentially I want to return the IntlConfig back as an object here. Debugging jest is causing me a bit of pain to figure it out 😅

My debugger is telling me this is returned as function but even when I add intlConfig() it's still returned as a function 😬 Is there something obvious I'm missing here?

config: { 'ember-intl.js': `module.exports = function() { return { wrapTranslationsWithNamespace: true } }`, },

Wow, it may be jest problem, if it's acting as a function, try to toString() it to figure out what is it.
In general, if tests is passing, it should work fine.
Provided config is correct.

patocallaghan · 2022-07-21T22:16:54Z

@lifeart I think we're good to go now. It turned out to be some transient issue with my development environment 🤷‍♂️ Restarting VSCode (to finish an update) and clearing the Jest cache meant my debugging and testing started working as expected 🤦‍♂️

So I've noticed even though I can commit to Aaron's branch I don't have permissions to move this PR out of "Draft" and also to update the PR description. Could you do that for me and also add the following as the PR description please?

Fixes #344

Ember Intl exposes a configuration file which allows you to configure the localization setup. One such setting is wrapTranslationsWithNamespace which determines whether translation keys should have the folder names within the key names.

`wrapTranslationsWithNamespace: false` (default)

# translations/sub-folder/en-us.yml
subFolder:
  foo: bar

this.intl.t('subFolder.foo');

`wrapTranslationsWithNamespace: true`

# translations/sub-folder/en-us.yml
subFolder:
  foo: bar

this.intl.t('sub-folder.subFolder.foo');

wrapTranslationsWithNamespace: true is not currently supported by this extension. This PR adds the ability, when wrapTranslationsWithNamespace: true is set in app/config/ember-intl.js, to support keys which have folders in their key names.

# [2.26.0](v2.25.0...v2.26.0) (2022-07-22) ### Features * 🧪 Support folder namespaces for intl ([#389](#389)) ([9099c60](9099c60))

patocallaghan · 2022-07-22T11:24:54Z

Just tested in our app and this works now! Thanks for the help @lifeart 👏

lifeart · 2022-07-22T11:46:07Z

@patocallaghan @achambers Thank you for your work and contribution!
uELS 3.0.33 released :)

lifeart · 2022-07-22T12:27:05Z

@patocallaghan could you check this behaviour on your project? lifeart/els-intl-addon#6 (comment)

patocallaghan · 2022-07-22T12:40:32Z

@lifeart looking 👀

patocallaghan · 2022-07-22T12:51:47Z

Yep I've noticed that intellisense doesn't work in all cases. Looking into it now.

@toovy you mention the absolute path is off for your examples. How do you see the path?

Also any chance you could pull together a test case using the test syntax similar to https://github.com/lifeart/ember-language-server/pull/389/files#diff-65bd0ca4089daf08ddfa3876799a64c1dc87e2629dfae17a16030a0c0b5aa8caR14 just so I can replicate what you are seeing?

toovy · 2022-07-22T13:05:09Z

@patocallaghan I try to be as helpful as I can, still, I'm pretty busy at the moment.

# Example from the translations/en-us.yaml
admin:
    countries: Countries

This is what it looks like when I use the intellisense when the string is still empty:

This is the result when e.g. choosing "Countries" (basically the complete path on my volume; correct would be {{t 'admin.countries'}}):

This is how our translations folder looks like:

No translation/no path outside the main YAML is taken into consideration for the intellisense at the moment.

BR Tobi

patocallaghan · 2022-07-22T13:06:29Z

Thanks @toovy 🙇‍♂️

patocallaghan · 2022-07-22T18:29:50Z

@toovy just curious but what editor are you using? I don't get any intellisense when it's {{t ""}} and also I can get my editor to show intellisense for translations in nested directories (although I've found some instances where it doesn't work but need to figure those out).

I might create a test app to verify because at the minute all the unit tests I write in this repo to find the bug pass 🤔

Edit: I'm assuming it's VS Code but thought I'd ask anyway as I get a different experience than you.

Edit: nvm found the bug 👍

patocallaghan · 2022-07-22T20:05:39Z

@toovy key name issue should be fixed by #392

Follow up to the bug found in #389 (comment) Make sure to generate the correct key name when the translation file as at the root translations directory, e.g. `translations/en-us.yml`. ### Before `.var.folders.mk.5t937px5627_y_j551f8gbkm0000gn.T.tmp-84101gxBh16jwQL7q.translations.admin.countries` ### After `admin.countries`

## [2.26.1](v2.26.0...v2.26.1) (2022-07-22) ### Bug Fixes * key name when translation file is at the root ([#392](#392)) ([4294adb](4294adb)), closes [/github.com//pull/389#issuecomment-1192553091](https://github.com//github.com/lifeart/ember-language-server/pull/389/issues/issuecomment-1192553091)

achambers · 2022-07-25T08:10:36Z

src/builtin-addons/core/intl-utils.ts

+      return null;
+    }
+
+    return typeof intlConfig === 'function' ? intlConfig() : intlConfig;


@patocallaghan I've been throwing this around in my head for a while. The config function takes an ember-cli environment argument. While we don't do this I wonder if there's a world where people set different config based on the environment. I wonder whether we should pass development in to the function just to be sure. Seeing as this is for intellisense, I think it's pretty safe to assume the ember-cli environment will be development at that point.

Thoughts?

(Did a code search and here's one such example. I know passing in development wouldn't affect the outcome here but it's an example of how the argument could be used.)

Yep, I'm up with specifying env = 'development' to intl config function, it could hep us implement #335

toovy · 2022-07-27T07:23:08Z

@toovy just curious but what editor are you using? I don't get any intellisense when it's {{t ""}} and also I can get my editor to show intellisense for translations in nested directories (although I've found some instances where it doesn't work but need to figure those out).

I might create a test app to verify because at the minute all the unit tests I write in this repo to find the bug pass 🤔

Edit: I'm assuming it's VS Code but thought I'd ask anyway as I get a different experience than you.

Edit: nvm found the bug 👍

Yes, VS Code, thanks for fixing this! What is the simplest way to try it out without waiting for the release?

Edit: Are there people using different IDEs at all? ;)

patocallaghan · 2022-07-27T09:33:20Z

@toovy This should be out already 👍 1728455

lifeart · 2022-07-27T10:48:59Z

yep, lifeart/vscode-ember@06a5d87

toovy · 2022-07-27T12:51:27Z

@patocallaghan fix worked, no weird paths any more :) still, no nested files are analysed on my VSCode

achambers added 2 commits June 9, 2022 15:47

WIP Support folder namespaces

93ce599

DELETE - Run one test

0e02097

achambers mentioned this pull request Jun 10, 2022

implement /config/intl.js execution to get intl configurations (translations location, nesting, default locations, etc) #344

Closed

lifeart mentioned this pull request Jul 14, 2022

Autocomplete for Nested Translations lifeart/els-intl-addon#6

Closed

Updated to check for Ember Intl configuration

44dcb97

patocallaghan added 3 commits July 20, 2022 15:08

Removed unused fixtures

1e3de09

Update tests and intl config logic

32062b0

Tweak test

3b40cc5

patocallaghan reviewed Jul 20, 2022

View reviewed changes

Fix tests

d1e2554

lifeart marked this pull request as ready for review July 22, 2022 10:18

lifeart changed the title ~~[EXPERIMENT] 🧪 Support folder namespaces~~ feat: 🧪 Support folder namespaces for intl Jul 22, 2022

lifeart merged commit 9099c60 into lifeart:component-context-info-origin Jul 22, 2022

lifeart pushed a commit that referenced this pull request Jul 22, 2022

chore(release): 2.26.0 [skip ci]

5c1ea9e

# [2.26.0](v2.25.0...v2.26.0) (2022-07-22) ### Features * 🧪 Support folder namespaces for intl ([#389](#389)) ([9099c60](9099c60))

patocallaghan deleted the bugfix/support-folder-namespaces branch July 22, 2022 11:14

patocallaghan mentioned this pull request Jul 22, 2022

fix: key name when translation file is at the root #392

Merged

achambers commented Jul 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🧪 Support folder namespaces for intl #389

feat: 🧪 Support folder namespaces for intl #389

achambers commented Jun 9, 2022 •

edited by lifeart

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading

lifeart commented Jul 14, 2022

patocallaghan commented Jul 15, 2022 •

edited

Loading

lifeart commented Jul 15, 2022 •

edited

Loading

patocallaghan commented Jul 15, 2022

patocallaghan Jul 20, 2022

lifeart Jul 21, 2022

patocallaghan commented Jul 21, 2022

patocallaghan commented Jul 22, 2022

lifeart commented Jul 22, 2022

lifeart commented Jul 22, 2022

patocallaghan commented Jul 22, 2022

patocallaghan commented Jul 22, 2022

toovy commented Jul 22, 2022 •

edited

Loading

patocallaghan commented Jul 22, 2022

patocallaghan commented Jul 22, 2022 •

edited

Loading

patocallaghan commented Jul 22, 2022

achambers Jul 25, 2022

lifeart Jul 25, 2022 •

edited

Loading

toovy commented Jul 27, 2022 •

edited

Loading

patocallaghan commented Jul 27, 2022

lifeart commented Jul 27, 2022

toovy commented Jul 27, 2022

feat: 🧪 Support folder namespaces for intl #389

feat: 🧪 Support folder namespaces for intl #389

Conversation

achambers commented Jun 9, 2022 • edited by lifeart Loading

wrapTranslationsWithNamespace: false (default)

wrapTranslationsWithNamespace: true

codecov bot commented Jun 9, 2022 • edited Loading

Codecov Report

lifeart commented Jul 14, 2022

patocallaghan commented Jul 15, 2022 • edited Loading

lifeart commented Jul 15, 2022 • edited Loading

patocallaghan commented Jul 15, 2022

patocallaghan Jul 20, 2022

Choose a reason for hiding this comment

lifeart Jul 21, 2022

Choose a reason for hiding this comment

patocallaghan commented Jul 21, 2022

wrapTranslationsWithNamespace: false (default)

wrapTranslationsWithNamespace: true

patocallaghan commented Jul 22, 2022

lifeart commented Jul 22, 2022

lifeart commented Jul 22, 2022

patocallaghan commented Jul 22, 2022

patocallaghan commented Jul 22, 2022

toovy commented Jul 22, 2022 • edited Loading

patocallaghan commented Jul 22, 2022

patocallaghan commented Jul 22, 2022 • edited Loading

patocallaghan commented Jul 22, 2022

achambers Jul 25, 2022

Choose a reason for hiding this comment

lifeart Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

toovy commented Jul 27, 2022 • edited Loading

patocallaghan commented Jul 27, 2022

lifeart commented Jul 27, 2022

toovy commented Jul 27, 2022

achambers commented Jun 9, 2022 •

edited by lifeart

Loading

`wrapTranslationsWithNamespace: false` (default)

`wrapTranslationsWithNamespace: true`

codecov bot commented Jun 9, 2022 •

edited

Loading

patocallaghan commented Jul 15, 2022 •

edited

Loading

lifeart commented Jul 15, 2022 •

edited

Loading

`wrapTranslationsWithNamespace: false` (default)

`wrapTranslationsWithNamespace: true`

toovy commented Jul 22, 2022 •

edited

Loading

patocallaghan commented Jul 22, 2022 •

edited

Loading

lifeart Jul 25, 2022 •

edited

Loading

toovy commented Jul 27, 2022 •

edited

Loading