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

RFC: Middleware #110

Open
gilbsgilbs opened this issue Dec 10, 2019 · 10 comments
Open

RFC: Middleware #110

gilbsgilbs opened this issue Dec 10, 2019 · 10 comments
Labels
enhancement New feature or request RFC Request for comments

Comments

@gilbsgilbs
Copy link
Owner

Is your feature request related to a problem? Please describe.

babel-plugin-i18next-extract is not very flexible and extensible right now. It cannot integrate easily with complex workflows and corner cases (e.g. monorepos ICU, custom extractors, complex combinations of files, you name it…). We need a way to write "addons" for this babel plugin.

Describe the solution you'd like

  • Module based
    So that addons are reusable. i.e. One could just yarn install --dev i18next-extract-plugin-icu and add some config to the babelrc to gain ICU support.
  • Custom extractors
    These extractors must be able to call and use one another.
    e.g. Custom Trans-like components
  • Custom exporters
    e.g. ICU or Fluent or legacy formats such as JSONv2
  • Optional: Support for custom comment hints
    e.g. Some exporter could now if // i18next-extract-some-icu-option-next-line foobar comment actually affects its current node.

TBD code examples.

Describe alternatives you've considered

  • A simpler approach that would be suitable in simple cases would be to allow writing addon functions in babelrc (as this config file can be written in JS as well).

    It would have some downsides:

    • not reusable
    • not modular (the user would have to write all the code in one file, not sure it even supports imports and modern ES syntax)
    • non-serializable configuration

    However it would be a very simple approach in comparison to forced modules. Users may be disappointed to find they have to write a whole module to integrate their workflow. Depending on the API, it may be possible to support both alternatives, but supporting the babelrc one must not lead to increased maintenance. In any case, we need recipes, good docs and templates.

@jolyndenning
Copy link
Contributor

Do you have an idea of what the API of the module would be? If so, could you share?

Having just created a PR in #112, I think the it might be rather tricky to create an API that is usable/useful for most cases. For example, the line between extracting keys and exporting gets rather thin. I originally thought that ICU would just be a different way of exporting the data, but it turns out that the format of the keys and values is created during the extraction phase, not the exporting phase. And that the extraction phase has multiple parts to it, each of which might need to be hookable. For example, in ICU you actually would want to prevent the derived keys for context and plurals to be generated at all.

@gilbsgilbs
Copy link
Owner Author

gilbsgilbs commented Dec 11, 2019

It is very possible that we have to rework how the extraction work quite deeply actually so that every aspect of the extraction is clearly defined. However, wouldn't the ICU use-case (and probably a lot of others) be solvable just by adding more context (aka metadata) to the keys, especially when computing derived keys? More specifically, computeDerivedKeys could add a plural count, a context, and possibly links (i.e. composition) between "derived keys" to the TranslationKeys. An ICU middleware could then just compute the keys by filtering out plural keys and reworking the singular key (as you did in your PR).

But your remark makes me think that the current API of exporters is very inappropriate for this purpose because it just exposes one addKey function which is too narrow. We either need to change the exporter API to be able to add multiple keys at once (=> so that derived keys are always added in batch <=) or to add some sort of intermediate pipeline between the extraction phase and the export phase. (Edit: one downside of doing it outside of exporters is that it would make it hard to write exporters for formats that are very context-dependant (such as Fluent). Exporters are already a bit awkward to work with because of the way Babel plugin API works.) For ICU, it could look like:

// @joeldenning/icu/index.js (pseudo-code)

function icuPipeline(config, locale, translationKeys) {
  // This function would be called once by batch of derived keys
  // e.g. [{key: "foo", …}, {key: "foo_plural", pluralCount: 1, …}]

  const underivedKeysWithPlurals = new Set(
    translationKeys.filter((k) => !!k.pluralCount).map((k) => k.derivedFrom)
  );

  /*
  well, you do whatever you want in the pipeline
  the only important thing is that you return an array of "TranslationKey"
  this way you can add/remove/modify keys
  */

  return underivedKeysWithPlurals.map(computeIcuValue);
}

export {
  "extractors": {},
  "pipelines": {
    "icu": icuPipeline,
  },
  "exporters": {},
}


// .babelrc
{
  "pipelines": [
    "@joeldenning/icu:icu"
  ]
}

ExtractedKeys and TranslationKeys could also include arbitrary metadata so that you could pass information from extractors/pipelines down to the exporter. One neat use-case I can think of: having a pipeline that finds comments around the key and passes them to an exporter that supports textual contexts (like .po):

// i18next-extract-comment-next-line Title of the contextual menu that the user sees when right-clicking blablabla…
<p>{t('contextualMenuTitle')}</p>

// Would be exported to .po:
//    #: foobar.js:38
//    #: Title of the contextual menu that the user sees when right-clicking blablabla…
//    msgid "contextualMenuTitle"
//    msgstr ""

I hope all this makes sense?

Also, I wanted to highlight that we might want to fix #87 (as well as #78?) first of all because it might affect the way we write extractors (resp exporters). Exposing such middleware API would freeze a lot of things, so I think we should make sure everything is stable first.

@gilbsgilbs
Copy link
Owner Author

💡 One specificity of extractors is that they have to deal with the AST. Therefore the middleware API has to provide a way to hook into this plugin's main visitor (currently in plugin.ts). I can't think of an elegant way to do this right now. We could decide to leave extractors out of the middleware API in a first iteration.

💡 I wonder if we should enforce some sort of prefix to the middleware modules published to NPM, the way Babel does for plugins (babel-plugin-xyz).

@jolyndenning
Copy link
Contributor

Thanks for the explanations and code example. So pipelines would be an in-between step between extraction and exporting?

What isn't clear to me in the pseudo code you wrote is how babel-plugin-i18next-extract would be used as a module. Would it be something like this, maybe?

// @joeldenning/icu
import { createBabelPlugin } from 'babel-plugin-i18next-extract'

const opts = {
  "extractors": {},
  "pipelines": {
    "icu": icuPipeline,
  },
  "exporters": {},
}

export default createBabelPlugin(opts);

@gilbsgilbs
Copy link
Owner Author

gilbsgilbs commented Dec 12, 2019

So pipelines would be an in-between step between extraction and exporting?

Exactly. I don't like the name pipeline much btw, but that's the idea.

What isn't clear to me in the pseudo code you wrote is how babel-plugin-i18next-extract would be used as a module. Would it be something like this, maybe?

In your snippet, what would the createBabelPlugin function do exactly? How would babel-plugin-i18next-extract invoke the icuPipeline function? I feel like createBabelPlugin is the identity function because we would get its return value through require() anyways?

// in babel-plugin-i18next somewhere:

const middleware = require('@joeldenning/i18next-extract-middleware-icu');

const newKeys = middleware.pipelines['icu'](config, locale, translationKeys);

(obviously, @joeldenning/i18next-extract-middleware-icu and icu strings would come from Babel config and wouldn't be hard-coded)

babel-plugin-18next-extract should however expose to middlewares some type definitions, the existing extractors and some of their helper functions (if we allow custom extractors in the first iteration), and maybe some other utils.

@jolyndenning
Copy link
Contributor

Ohh I see now. I misread your earlier example babelrc:

{
  "pipelines": [
    "@joeldenning/icu:icu"
  ]
}

I was thinking that icu would be a babel plugin itself. And that you'd use that plugin instead of using the babel-plugin-18next-extract plugin. Now I think I see what you're going for.

Doesn't babel configuration for a plugin go directly onto the plugin, instead of just plopped onto the babelrc root object? In other words, shouldn't it be this?

.babelrc

{
  "plugins": [
    ["babel-plugin-i18next-extract", {
      "pipelines": ["@joeldenning/icu:icu"]
    }]
  ]
}

What I was thinking earlier was that babel-plugin-i18next-extract would expose an api for creating a customized version of the babel plugin:

babelrc:

{
  "plugins": ["babel-plugin-i8next-icu-extract"]
}

@gilbsgilbs
Copy link
Owner Author

Doesn't babel configuration for a plugin go directly onto the plugin, instead of just plopped onto the babelrc root object? In other words, shouldn't it be this?

Oh yeah, sorry about that, you are absolutely right. It was clear in my head that I was talking about the plugin config. I wanted to avoid writing too much config for the sake of keeping the example readable and concise, but it eventually led to even more confusion.

What I was thinking earlier was that babel-plugin-i18next-extract would expose an api for creating a customized version of the babel plugin:

Actually, this could be a way around the problem of writing extractors. I can't say I like it though.

@jolyndenning
Copy link
Contributor

I’m okay with either approach. If it’s the config approach, I think one simplification could be to not specify pipelines, extractors, and exporters as config values in the babelrc, but rather just have a plugins array with a list of modules to use as plugins. Each plugin module would by convention export those things with the correct name. Example of what I’m thinking:

{
  "plugins": [
    ["babel-plugin-i18next-extract", {
      "plugins": ["@joeldenning/icu"]
    }]
  ]
}

Additionally, I don’t see any downsides to the alternative proposal that makes it possible to create your own Babel plugin using the js api for i18next-extract. What don’t you like about it?

Last thing, what are your thoughts on #112? My preference is actually towards merging that PR instead of creating and maintaining my own plugin for ICU. However, I’m open to whichever option you think is best.

@pcorpet
Copy link
Contributor

pcorpet commented Mar 21, 2020

Any progress here?

@gilbsgilbs
Copy link
Owner Author

@pcorpet Not yet unfortunately. I haven't been able to spend a single minute on the project for a while, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request RFC Request for comments
Projects
None yet
Development

No branches or pull requests

3 participants