fix(common): duplicate translation namespace prefix, fixes #738

[someusersomeuser]

No description provided.

[codecov]

Codecov Report

Merging #739 (330cf3c) into master (53eddaf) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #739   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          233       233           
  Lines        16439     16438    -1     
  Branches      5490      5490           
=========================================
- Hits         16439     16438    -1     

Impacted Files	Coverage Δ
packages/common/src/extensions/extensionUtility.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ghiscoding]

I took a quick look at this change and where this function is called, so it might fix most of them but that would probably break the Cell Menu because that seems to be the only one that doesn't have predefined commands and so we cannot add the prefix. The other problem that this will potentially break would be when the user adds a custom command for any of these plugins as for example Grid Menu, then the command will not get the prefix... so I think that the only solution is probably the inverse which would be to remove leave the prefix in this method but remove it everywhere else (Grid Menu, Context Menu, etc...). I know this would require more code change but is still probably better because we still want the prefix to be added even on custom menu commands

[someusersomeuser]

I think this is the correct behaviour. if the user adds their own menu item, then the translation will be in the default space, if it is not, he can explicitly specify a different one.

[ghiscoding]

I kinda disagree, if there's a translation prefix then it should all or nothing. I don't think requiring the user to explicit add the prefix when he already added it in the i18n is the correct way, again I think it should be all or nothing and I would prefer to go with the last post I wrote

[someusersomeuser]

in this case user need copy and patch standart locale files to own project after each version upgrade.
otherwise, user add custom items in default locale file 'translation.json', or for example 'aurelia-slickgrid-custom.json'

[ghiscoding]

Like I said I never used these prefix and/or i18n workspace but I'm getting confused

Let say that we have a workspace prefix of aurelia-slickgrid and the user adds Grid Menu (or any other menu) commands like this

this.gridOptions = {
    gridMenu: {
      // custom commands
      commandItems: [
        { command: 'help', titleKey: 'HELP', positionOrder: 70, action: (e, args) => console.log(args) },
      ],
    },
}

and slickgrid-universal internal commands like this with the prefix

// internal commands
gridMenuCommandItems.push(
            {
              iconCssClass: this._gridMenuOptions.iconClearAllFiltersCommand || 'fa fa-filter text-danger',
              titleKey: `${translationPrefix}${commandLabels?.clearAllFiltersCommandKey ?? 'CLEAR_ALL_FILTERS'}`,
              // will be transformed into:  titleKey: `aurelia-slickgridCLEAR_ALL_FILTERS`
              disabled: false,
              command: commandName,
              positionOrder: 50
            }
);

this mean that you'll have the slickgrid internal translations with the aurelia-slickgrid prefix and the user custom translations without the prefix.

allCommandItems = [
  // internal commands
  { command: 'help', titleKey: 'aurelia-slickgridCLEAR_ALL_FILTERS', positionOrder: 70, action: (e, args) => console.log(args) },

  // custom commands
  { command: 'help', titleKey: 'HELP', positionOrder: 70, action: (e, args) => console.log(args) },
}

Is that really what you're expecting? Because that wasn't at all my expectation of this prefix and conflicts with me definition of the translation prefix. I mean that I know you're trying to fix the i18n workspace but my conception of the prefix is again defined as an all or nothing (not a partial of it)

[someusersomeuser]

yes.

  aurelia.use.plugin(PLATFORM.moduleName('aurelia-i18n'), (instance) => {
    const aliases = ['t', 'i18n'];
    TCustomAttribute.configureAliases(aliases);
    instance.i18next.use(Backend);
    return instance.setup({
      backend: { loadPath: './locales/{{lng}}/{{ns}}.json' },
      attributes: aliases,
      lng: 'en',
      ns: ['translation', 'aurelia-slickgrid'],
      defaultNS: 'translation',
      fallbackLng: 'en',
      debug: false
    });

'aurelia-slickgrid' - from npm with standart keys as is
'translation' - this is all user keys for application - buttons, messages, etc, including custom grid menu items.
default namespace, prefix dont need

allCommandItems = [
  // internal commands
  { command: 'help', titleKey: 'aurelia-slickgrid:CLEAR_ALL_FILTERS', positionOrder: 70, action: (e, args) => console.log(args) },

  // custom commands
  { command: 'help', titleKey: 'HELP', positionOrder: 70, action: (e, args) => alert(this.i18n.tr('HELLO_WORLD')) },

  { command: 'refresh', titleKey: 'slickgrid-custom-menu-section-from-translation.REFRESH_DATA', positionOrder: 70, action: (e, args) => alert(this.i18n.tr('other-user-section-from-translation.messages.REFRESH_COMPLETED')) },
}

first item with namesace and ':' separator (by default)
other keys in user workspace without namespace, but can contains sections

[ghiscoding]

yeah ok I checked today and considering that I named the grid option as translationNamespace, then I guess your point of view is acceptable. I assume that you managed to test this? If not you can modify the file from node_modules/@slickgrid-universal/common or run the bundle command and replace the same node_modules folder

[ghiscoding]

@someusersomeuser
Thanks for the PR, this was released under new version v1.4.0 and also a new version of Aurelia-Slickgrid, thanks again for the contribution 😉