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

Feat/locale #65

Closed
wants to merge 8 commits into from
Closed

Feat/locale #65

wants to merge 8 commits into from

Conversation

shhdharmen
Copy link
Contributor

@shhdharmen shhdharmen commented Jan 18, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1

What is the new behavior?

1. Translation languages

Add languages in packages/falso/project.json's translate executor's options.

2. Generate translation files

npm run translate

Above command will...

  1. Translate texts from json for that particular language (API implementation is remaining)
  2. Create language specific files in below structure:

image

2. Add configuration for each language in rollup.config.js:

import json from '@rollup/plugin-json';

export default [
  {
    input: 'dist/out-tsc/packages/falso/src/i18n/es/index.js',
    output: {
      file: 'dist/packages/falso/i18n/es/index.cjs.js',
      format: 'cjs',
    },
    plugins: [json()],
  },
  {
    input: 'dist/out-tsc/packages/falso/src/i18n/es/index.js',
    output: {
      file: 'dist/packages/falso/i18n/es/index.esm.js',
      format: 'esm',
    },
    plugins: [json()],
  }
];

Above approach can be simplified using glob patterns, but it requires more research.

3. Build the library with secondary entry points for each language

npm run build

Above command will...

  1. Build main falso library without i18n
  2. run the postBuild.js script, which will internally run all needed scripts to create entry points for each language.

3. Usage

After packing and publishing the library, one can use the translation like below:

const { randAbbreviation } = require("@ngneat/falso/i18n/es");

// or

import { randAbbreviation } from '@ngneat/falso/i18n/es';

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@shhdharmen shhdharmen marked this pull request as draft January 18, 2022 19:40
@NetanelBasal
Copy link
Member

Thanks. Why do we need to copy the core?

@NetanelBasal
Copy link
Member

Also, does that mean each package should be installed via npm? That's not was the intention :)

@michaelxvoelker
Copy link
Contributor

I think the translation should generate submodules with its own entry-points within packages/falso/src and bundled all together in one package. Maybe this might help to get the correct setup.

@shhdharmen
Copy link
Contributor Author

Also, does that mean each package should be installed via npm? That's not was the intention :)

@NetanelBasal no, user just need to install falso, i18n files are part of it.

@shhdharmen
Copy link
Contributor Author

Thanks. Why do we need to copy the core?

@NetanelBasal because without copying, when end user tries to access any function from i18n, it gives error of core functions not found. The reason behind that is lib is bundled separately and i18n files are bundled separately.

Although, I will try to find out a way to eliminate that.

@shhdharmen
Copy link
Contributor Author

I think the translation should generate submodules with its own entry-points within packages/falso/src and bundled all together in one package. Maybe this might help to get the correct setup.

Thanks for sharing. Initial structure was like that only, but nx only bundled main index.ts file. I will check if that can be changed through some configs.

@michaelxvoelker
Copy link
Contributor

Looks tricky to setup correctly. I wonder if the rollup plugin for multiple entry files can do the trick. 🤔

@shhdharmen
Copy link
Contributor Author

@NetanelBasal modified the code which doesn't require copying core files. Let me know if this is good, then I can start looking into API integration.

packages/falso/package.json Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
tools/executors/translate/index.ts Show resolved Hide resolved
tools/executors/translate/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved

const data = jsonData.data;

const updatedData = await translateDataArray(data, language);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need all this code? you have the data array property from the JSON file, and we should translate its elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed for nested jsons.

Copy link
Member

Choose a reason for hiding this comment

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

For example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example data is array of objects instead of strings.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have such a case. Our data is always a list of strings

Copy link
Member

Choose a reason for hiding this comment

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

@shhdharmen now I see the food example, you are right 😀

Copy link
Member

Choose a reason for hiding this comment

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

Chexk if you support this please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked for food, worked fine.

@@ -154,23 +77,29 @@ export function createLanguageIndexFile(outputDir: string) {

readdirSync(outputDir)
Copy link
Member

Choose a reason for hiding this comment

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

use glob

tools/executors/translate/write.ts Show resolved Hide resolved
@@ -34,5 +34,8 @@
"devDependencies": {
"@types/seedrandom": "3.0.1",
"@types/uuid": "8.3.4"
},
"exports": {
"./i18n/*": "./i18n/*/index.cjs.js"
Copy link
Member

Choose a reason for hiding this comment

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

Can you verify it works with web bundlers such as Wepback, please? I' suggesting to build the library and npm link to dist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked for webpack, it gave error for missing entry of . in exports, so added that and it worked fine.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this approach because you're referring only to cjs modules here, and web bundles can't tree-shake it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other approach could be to have package.json for each language, and it contains main and module entries in it and remove exports from main package.json. Should I check that? We will need to take care that users still need to only install main package only.

Or can we use other builder than rollup? Like node? Which supports multiple entry points: https://nx.dev/node/build#additionalentrypoints. And it's built on top of webpack, which can help us with more configurations.

Copy link
Member

Choose a reason for hiding this comment

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

We will need to ensure that users still need to install the main package only.

That's not an issue because it's tree-shakable. It'll not be bundled if they do not use the i18n functions.

There's a problem we did not think of - name collision. We should prefix the i18n function with the locale (e.g. ruRandFullName)

Let's try the other builder with the entry point support regarding the build.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the prefix in place, we can just export the i18n functions in the regular index.ts file, right? 🤔 No additional entry point is needed for packaging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelxvoelker feel free to clone the repo or even the branch I created and try out what you mentioned. Let me know if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried node builder, but doesn't produce results the way we want. I will research more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants