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

Feature/default ts translations #38

Merged
merged 13 commits into from May 7, 2019

Conversation

Projects
None yet
2 participants
@baflo
Copy link
Contributor

commented Dec 20, 2018

Lädt alle Locales (translations, utterances, entities) aus dem in der Config angegebenen Order. Es wird ein Objekt erzeugt, das der Dateistruktur entspricht. Dazu wird folgende Logik angewandt:

  • Das Root Verzeichnis mit all seinen Dateien und Ordnern wird importiert
  • Basisdateinamen (ohne Endung) bzw. Ordnernamen werden als Keys im Objekt verwendet
  • Dateien in Unterordnern befinden sich auch in Subobjekten
  • Dateien und Ordner mit identischem Namen (ohne Endung) werden gemergt, wobei die Datei Priorität hat
  • Haben Dateien einen default export wird nur dieser extrahiert, dafür aber "transparent"
  • Haben Dateien keinen default export, besitzen jedoch einen export mit dem Dateinamen (oder camelcased), wird nur dieser exportiert, dafür aber "transparent"
  • Exportiert die Datei weder default noch den Dateinamen wird alles andere exportiert.

Beispiel:

Die Ergebnisse aus translation.ts und dem Ordner translation werden zu einem Objekt gemergt. main-state.ts und translation.ts exportieren ihre Objekte unmittelbar, da sie entweder einen default export verwenden oder den Dateinamen. Letzteres erlaubt eine Typzuweisung ohne Casting. In Fall von utterances.ts werden alles exports tatsächlich exportiert, da weder der Dateiname noch ein default export Verwendung finden.

./locales/en/translation.ts

export default { 
    fallbackResponse: []
}

./locales/en/translation/main-state.ts

export const mainState = {
  invokeGenericIntent: []
}

./locales/en/utterances.ts

export const invokeGenericIntent = [];

Locales Objekt:

{
  en: {
    translation: {
      fallbackResponse: [],
      mainState: {
        invokeGenericIntent: []
      }
    },
    utterances: {
      invokeGenericIntent: []
    }
  }
}

@baflo baflo requested a review from antoniusostermann Dec 20, 2018

@antoniusostermann antoniusostermann changed the base branch from feature/improved-test-setup to develop Jan 3, 2019

@antoniusostermann

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

changed base to develop after merging feature/improved-test-setup

@antoniusostermann antoniusostermann removed their request for review Mar 8, 2019

@baflo baflo force-pushed the feature/default-ts-translations branch from be9f370 to a2f978e Apr 4, 2019

@baflo baflo requested a review from antoniusostermann Apr 4, 2019

src/cli.ts Outdated
// Create base TypesScript files
[
{ filePath: "config/locales/en/translation/main-state.ts", exportString: "{}" },
{ filePath: "config/locales/en/utterances/invokeGenericIntent.ts", exportString: "[]" },

This comment has been minimized.

Copy link
@antoniusostermann

antoniusostermann Apr 18, 2019

Collaborator

please align this file path to our regular conventions. You should not use camelCase in path strings ("invoke-generic-intent.ts" instead of "invokeGenericIntent.ts").

// Use cached data
return this.entities;
return this.locales;

This comment has been minimized.

Copy link
@antoniusostermann

antoniusostermann Apr 18, 2019

Collaborator

Please add a comment over here why this caching is so important (blockig io etc). Should we store the locales in a static variable?

This comment has been minimized.

Copy link
@baflo

baflo Apr 26, 2019

Author Contributor

I thought this through and came to a couple of conclusions.

  1. If locales was static, the only instance member left in this class would be the unifier configuration.
  2. It would be kind of odd if we need the unifier configuration to fill a static variable.
  3. However, in contrast to the current setting, having the locales in a static variable would give a huge performance gain, as the locales loader is probably instantiated quite often, and hence the files are re-read very often.
  4. It's still useful to have the unifier configuration on each instance, as we may have a feature in future that allows to change the configuration on runtime.
  5. If the configuration changes, we might want to reload the locales.
  6. However, we weren't able to see file changes but only changes in the configuration (and hence the locales path)

In conclusion, I think the current setting is a good compromise of being able to update locales with each request and not loading them too often (in the limit once per request).

return result;
}

private static hasShadowingFile(absPath: string) {

This comment has been minimized.

Copy link
@antoniusostermann

antoniusostermann Apr 18, 2019

Collaborator

Did you touch the localeloader's spec files? Seems like you added a lot of new behaviour to it.

This comment has been minimized.

Copy link
@baflo

baflo Apr 26, 2019

Author Contributor

I think the behaviour you mean here is already being tested here, however I explicitely added a notion that TS also shadows JS and JSON.

const fn = path.basename(absPath);

// Extract only `default` or, if not available, object with camelcased name of file
let result = obj.default || obj[LocalesLoader.camelcase(path.basename(fn, path.extname(fn)))];

This comment has been minimized.

Copy link
@antoniusostermann

antoniusostermann Apr 18, 2019

Collaborator

this probably won't work anymore with normalized file names / needs some changes?

This comment has been minimized.

Copy link
@baflo

baflo Apr 26, 2019

Author Contributor

Still works :)

CHANGELOG Outdated
@@ -4,6 +4,7 @@ version 0.5.0
- adds option to UniferConfiguration that allows to change the way how request extractors are found.
- adds option to pass component's defaultConfiguration a function that returns a new configuration object
- passes list of components to except from autobind to inversify-components
- loads all TypeScript, JavaScript and JSON files from locales directory, if not i18next backend is configured

This comment has been minimized.

Copy link
@antoniusostermann

antoniusostermann Apr 18, 2019

Collaborator

Could you please add some more information about the ordering of loading and all possibilites (one file per state, on file per intent, folders, ...). Since we don't have docs currently, that info would be probably lost otherwise.

@baflo baflo requested a review from antoniusostermann Apr 26, 2019

baflo added some commits May 6, 2019

@baflo baflo requested a review from antoniusostermann May 7, 2019

@antoniusostermann antoniusostermann merged commit c35dc2b into develop May 7, 2019

3 checks passed

codeclimate 2 fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@antoniusostermann antoniusostermann deleted the feature/default-ts-translations branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.