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(format): initial support for external formatters #1511

Merged
merged 10 commits into from
Mar 16, 2023

Conversation

timofei-iatsenko
Copy link
Collaborator

@timofei-iatsenko timofei-iatsenko commented Mar 13, 2023

Description

Preparation for external formatters.

Currently, all extractors required in the CLI eagerly. It means no matter do you use CSV or po-gettext their dependency are loaded to the memory and affect cold-start time of CLI command. (and actually all bundler loaders plugins as well)

So the first point was to make them loading lazily.

This quickly revealed a problem with mock-fs in tests.

If during execution of a command require() calls happened, they would fail because FS is mocked and no such file to require found. So it's breaking the code.

In this PR i got rid of mock-fs in tests which were affected by this problem.

And add initial support for external formatters.

Notes:
Before we can release "externalFormatters" for public it's important to make formatters work in the async context. That will change their shape, obviously. I'm going to do this in next iteration to keep this PR small.

Docs would be written when final shape of formatter would stabilized.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Fixes # (issue)

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Mar 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 16, 2023 at 11:27AM (UTC)

@github-actions
Copy link

github-actions bot commented Mar 13, 2023

size-limit report 📦

Path Size
./packages/core/build/esm/index.js 1.57 KB (0%)
./packages/detect-locale/build/esm/index.js 812 B (0%)
./packages/react/build/esm/index.js 1.67 KB (0%)
./packages/remote-loader/build/esm/index.js 7.25 KB (0%)

@timofei-iatsenko timofei-iatsenko changed the title Refactor/formatters feature(format): support for external formatters Mar 14, 2023
@timofei-iatsenko timofei-iatsenko changed the title feature(format): support for external formatters feat(format): support for external formatters Mar 14, 2023
@timofei-iatsenko timofei-iatsenko changed the title feat(format): support for external formatters feat(format): initial support for external formatters Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Patch coverage: 79.45% and project coverage change: +1.20 🎉

Comparison is base (7fc9dee) 74.55% compared to head (85ab0c9) 75.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1511      +/-   ##
==========================================
+ Coverage   74.55%   75.75%   +1.20%     
==========================================
  Files          76       79       +3     
  Lines        1965     1980      +15     
  Branches      515      516       +1     
==========================================
+ Hits         1465     1500      +35     
+ Misses        389      360      -29     
- Partials      111      120       +9     
Impacted Files Coverage Δ
packages/cli/src/lingui-compile.ts 30.00% <16.66%> (-0.67%) ⬇️
packages/cli/src/api/formats/index.ts 41.66% <50.00%> (-18.34%) ⬇️
packages/cli/src/api/formats/minimal.ts 62.50% <60.00%> (+44.85%) ⬆️
packages/loader/test/compiler.ts 66.66% <66.66%> (ø)
packages/cli/src/api/formats/po-gettext.ts 76.00% <75.00%> (+1.96%) ⬆️
packages/cli/src/api/formats/lingui.ts 88.23% <84.61%> (-2.25%) ⬇️
packages/cli/src/api/catalog.ts 91.91% <100.00%> (+1.29%) ⬆️
packages/cli/src/api/catalog/getCatalogs.ts 93.61% <100.00%> (ø)
...s/cli/src/api/catalog/getTranslationsForCatalog.ts 96.42% <100.00%> (ø)
...ckages/cli/src/api/formats/api/formatterWrapper.ts 100.00% <100.00%> (ø)
... and 11 more

... and 1 file with indirect coverage changes

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review March 14, 2023 11:02
@andrii-bodnar
Copy link
Contributor

@thekip thank you! How do you see the implementation of new formats?

It would be great if it allow the creation of separate shareable packages for different formats that can be installed via npm and then used by Lingui. In that way, every developer will be able to implement their own formatter and share it with other developers.

@timofei-iatsenko
Copy link
Collaborator Author

How do you see the implementation of new formats?

Exactly like you described.

Even more, i'm going to extract all existing formats from @lingui/cli and make them available as separate packages.

The only @lingui/formatter-po would be included in @lingui/cli dependencies, for the rest user would have to install them first.

This is slightly reduce installation size, because if you don't use CSV formatter you don't have to install papaparse.

It also will help to gather some kinda statistics which packages people uses and which not.

For migration period there would be a warning

// lingui.config.js
{
  format: 'minimal'
}

Error: Formatter minimal is no longer included by default. You have to install it with npm -D @lingui/formatter-minimal

Something like this.

Also if you want to specify some formatter parameters you have to do it in the factory function instead of formatterOptions. This should reduce maintenance burden of current formatters.

// lingui.config.js
import formatter from "@lingui/formatter-minimal"
{
  locales: ['en', 'pl'],
  format: formatter({myAwesomeOption: true})
}

@andrii-bodnar
Copy link
Contributor

Sounds great! I think formatters should have access to as much contextual information about the string as possible. If needed, it will allow the generation of quality localization files with all the possible context for translators

@timofei-iatsenko
Copy link
Collaborator Author

Made formatters work in the async context. (check last commit)
To achieve async context in formatters, it was necessary to make the whole call tree up to the command async as well.

Also reanimated and fixed tests for webpack loader, which was ignored.

@timofei-iatsenko
Copy link
Collaborator Author

Looking into current formatters api i'm wondering about correctness and responsibilities.

Currently, the api is read(filename) and write(filename) (simplified).

It means that each formatter is responsible for two things:

  • Parsing/serializing of format, for example js data -> .po or js data -> json
  • And working with FS. It means formatters go to FS by it's own, and reading/writing part is mostly duplicated between all formatters.

I think it might be incorrect. Responsibility of formatter should be only first point from the list.

So API should looks like as parse(string | buffer): Catalog and serialize(catalog: Catalog): string | buffer

This way all heavy lifting related to work with a FS would be out of the formatters reponsibilities.

@andrii-bodnar what do you think?

@timofei-iatsenko
Copy link
Collaborator Author

timofei-iatsenko commented Mar 15, 2023

But there are some tricky logic in write functions in the formatters, which makes this mentioned idea a bit hard to implement:

// po formatter
    async write(filename, catalog, ctx) {
      let po: PO

      const raw = await readFile(filename)
      if (raw) {
        po = PO.parse(raw)
      } else {
        po = new PO()
        po.headers = getCreateHeaders(ctx.locale)
        if (ctx.locale === undefined) {
          delete po.headers.Language
        }
        // accessing private property
        ;(po as any).headerOrder = Object.keys(po.headers)
      }
      po.items = serialize(catalog, options)
      await writeFileIfChanged(filename, po.toString())
    },

or

      const messages = serialize(catalog)
      let file = await readFile(filename)

      const shouldUseTrailingNewline = file === null || file?.endsWith("\n")
      const trailingNewLine = shouldUseTrailingNewline ? "\n" : ""
      await writeFile(
        filename,
        `${JSON.stringify(messages, null, 2)}${trailingNewLine}`
      )

Anyway, i'm done with this PR.
In follow up PRs i will extract formatters from @lingui/cli, add deprecation for @lingui/conf parameters and add docs.

@andrii-bodnar
Copy link
Contributor

@thekip I also think that working with FS definitely is not formatter's responsibility. Ideally, the formatter should be responsible only for parsing and serializing, and reading/writing should happen outside.

This will also reduce code duplication and the developer of the custom formatter will not care about the FS, only for the parsing and building of the file.

@andrii-bodnar andrii-bodnar added the ↻ awaiting rebase Rebase is needed due to conflicts label Mar 15, 2023
@timofei-iatsenko
Copy link
Collaborator Author

@andrii-bodnar please, merge

@timofei-iatsenko
Copy link
Collaborator Author

Finally, was able to make it (see last commit):

I also think that working with FS definitely is not formatter's responsibility. Ideally, the formatter should be responsible only for parsing and serializing, and reading/writing should happen outside.

This will also reduce code duplication and the developer of the custom formatter will not care about the FS, only for the parsing and building of the file.

@andrii-bodnar
Copy link
Contributor

I'd like to review it in more detail, will merge it tomorrow

@timofei-iatsenko
Copy link
Collaborator Author

test suite should be rerun

@timofei-iatsenko
Copy link
Collaborator Author

kinda flaky test, looks like depending on something in host system glob may return results in one order or another. That will produce slightly different cli output which is not matched to snapshot

@timofei-iatsenko
Copy link
Collaborator Author

fixed flaky test

@andrii-bodnar andrii-bodnar merged commit 4f1d30e into lingui:next Mar 16, 2023
@timofei-iatsenko timofei-iatsenko deleted the refactor/formatters branch March 16, 2023 12:46
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.

2 participants