gilbsgilbs / babel-plugin-i18next-extract

Babel plugin that statically extracts i18next and react-i18next translation keys.
https://i18next-extract.netlify.com
MIT License
161 stars 37 forks source link

Allow fallback to importDeclaration if bindings are not up to date #60

Open redbugz opened 5 years ago

redbugz commented 5 years ago

We are using ICU message format in our JSON files, and thus using the babel macros exported by react-i18next/icu.macro (Trans (different), Select, SelectOrdinal, Plural). https://react.i18next.com/misc/using-with-icu-format

However, those macros do not properly set up the bindings in the AST correctly after swapping out the macro imports for the "normal" react-i18next Trans import, so this plugin doesn't recognize the new Trans import and doesn't extract the translations (isTransComponent and referencesImport return false because they can't find a valid binding)

I have learned my way around babel plugins and macros enough to fix some issues with the ICU macros, so that they generate the correct source code, but I've tried and tried to fix the bindings and I just can't figure out how to do it properly in the AST.

I have a workaround in referencesImport that adds an additional fallback if the bindings don't exist properly, to just look up the import in the ImportDeclarations. Would that be a welcome PR? The other alternative is to get some help on making the ICU macro correctly update the bindings when it transforms the source code.

A bit of backstory if helpful: I've been trying to use other command line extractors, but they choke on the curly braces and comma separated options in the ICU format, and they are just doing static parsing with acorn, so they can't take advantage of the work already in the ICU babel macro. And teaching them to recognize the ICU format seems like a lot of work. Once I found this project, I realized how much cleaner it would be if the macro could run first, transform the source to handle the funky ICU format curly braces, then the extractor wouldn't need to know any special format, and would just have a "normal" Trans element to work with.

I seem to have that working locally as long as I have the workaround to fallback to direct inspection of the ImportDeclarations if a suitable binding can't be found.

I can provide more details or examples if that is helpful.

What solution do you think would be best for this type of situation?

gilbsgilbs commented 5 years ago

Hi, Thanks for raising an issue.

I don't know anything about ICU and I'll have to read more about it and how it integrates with i18next before I can give you a relevant answer.

About the detection of Trans components that are imported from the macro, maybe #33 could help, but if we don't want to wait for it we could also add macro imports in the existing whitelist. I'm not sure it would really do what you expect in any case: it would consider the Trans component from the macro to be a normal Trans component. I reckon we're looking for something slightly more subtle than that, isn't it?

Once I found this project, I realized how much cleaner it would be if the macro could run first, transform the source to handle the funky ICU format curly braces, then the extractor wouldn't need to know any special format, and would just have a "normal" Trans element to work with.

This is in theory. In practice, I don't think Babel gives you much control over when transformations happen. And you might get bitten by some other transformation if it happens too early (e.g. JSX). That's why the plugin currently runs on the Program node (the root node) and traverses the untransformed AST from there (to ensure no transformation has taken place). I'm definitely not against changing this (since it is terrible performance-wise), yet we have to be very careful about it.

I'm also considering allowing third-party plugins (extractors and exporters) at some point (probably not really short term though). Maybe ICU could be a good use-case for a custom exporter.

Sorry if I missed the point. I'm not against some more examples so that I can have the bigger picture.

gilbsgilbs commented 5 years ago

I have a workaround in referencesImport that adds an additional fallback if the bindings don't exist properly, to just look up the import in the ImportDeclarations. Would that be a welcome PR?

Do you mean adding || referencesImport(openingElement.get('name'), 'react-i18next/icu.macro', 'Trans') to isTransComponent function? Do you have a fork where I could see what you've done? I don't think I have anything against adding basic support for components from the macro, as long as it doesn't require very deep complications to the code.

redbugz commented 5 years ago

I was just adding this in referencesImport before the last return false:

  const existingImport = nodePath.hub.file.path.node.body.find(
      importNode => BabelTypes.isImportDeclaration(importNode) && importNode.source.value === 'react-i18next',
  );
  if (existingImport) return true;

  return false;

It's failing some tests in the repo so I have to figure that out, but it's working in my app. And I can probably tighten up the checks to be a bit more targeted.

I'm running the plugin like this: package.json:

    "locales:extract": "NODE_ENV=development babel --config-file ./i18next-extract-babel.js 'src/**/*.js'",

i18next-extract-babel.js:

module.exports = {
  presets: ['react-app'],
  plugins: [
    'macros',
    [
      'i18next-extract',
      {
        keySeparator: null,
      },
    ],
  ],
}