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

refactor(macro): macro to plugin #1867

Merged
merged 16 commits into from Mar 6, 2024

Conversation

thekip
Copy link
Collaborator

@thekip thekip commented Feb 28, 2024

Description

There are few noticeable changes in this PR:

  1. Macro is re-written as regular babel plugin and hooked up to babel-macro-plugin
  2. Babel's scopes functionality is used to determine references between macro imports and usages
  3. Runtime imports inserted taking to account variables in current and all children scopes and should not make any collisions with existing bindings.

These changes make a transformation more robust, reliable and ready for more complicated cases which may appear for example after code bundling.

The original motivation was to avoid complications related to babel-macros architecture, but that it turned out that it will help with fixing few of the opened bugs (#1844 #1848 ) and fix broken experimental deps extractor

The problem with babel-macro-plugin for lingui macro:

Macro written for babel-macro-plugin works a little bit different way then usual babel plugins. Instead of defining a Visitor (object which enters each node of AST) it receives a list of references for nodes used from the macro package.

import {t, plural} from "@lingui/macro";

t`Message`
t`Hello ${plural()}`

For this snippet babel-macro-plugin will invoke our macro function one time with an object:

{
  t: [Node, Node],
  plural: [Node]
}

Then our macro should take each node and somehow process it.
The problem here lays in the nested macro calls:

t`Hello ${plural()}`

Which should be turned in a single i18n call:

i18n._("Hello {plural, ...}")

We receive 2 nodes for this one expression one node in t key another in plural, and we don't have any information about how these nodes are correlated to each other.

The previous macro used quite simple approach, we collect all nodes from all keys into one single array

const references = {
  t: [Node, Node],
  plural: [Node]
}
const jsNodes = [...references.t, ...references.plural]

And then add a condition before we start processing each node which recursively goes and check do each node has parent declared in this array. Let's call this "parent matching method"

jsNodes.forEach((node) => {
   if (!node.findParent((parent) => jsNodes.includes(parent))) {
     // transform node, only if its "root" node, not a nested node in another macro
   }
})

And this worked up to the point. The code inside transformation was written "imperatively", every possible case is handled manually by additional conditions in the flow. Although there are still cases which were not anticipated in the transformation code, such as:

t`Field ${t`First Name`} is required`

But since it's a rear case that wasn't a problem for a long time.

However, things start biting again during implementation of useLingui hook:

import {useLingui, plural} from "@lingui/macro"
const {t} = useLingui()
t`Hello ${plural()}`

Only useLingui and plural are imported from @lingui/macro, and we have only theirs nodes in the references, all usages of t are in the "gray" zone for the macro.

const references = {
  useLingui: [Node],
  plural: [Node],
}

So when we have a nested macro call it get transformed by it's own, not as part of outer expression.

This is solvable, and was solved in the original PR, but this feels bad, its fragile, hard to understand and probably not very performant (i didn't test though).

When things start complicating

So far, i covered simple, human written code. But that's not all.

Imagine the case:

import { t } from "@lingui/macro";
import { plural } from "@lingui/macro";

t`Hello ${plural()}`

This a completely valid code which may be even written by a person. Due to how babel-macro-plugin works our macro function will be called 2 times (for each import statement) with references belonging to each import:

// first invocation
const references = {
  t: [Node],
}

// second invocation
const references = {
  plural: [Node]
}

If you followed all my writings you can already guess, that our "parent matching method" described above will not work, since each invocation would have it's own array of nodes and mixing and matching macros referenced from one import with another would be treat as standalone macro calls and wouldn't be turned in one runtime i18n statement.

import { i18n } from "@lingui/core";

// would print 2 separate i18n statements
i18n._({
  message: 'Hello {0}',
  variables: {
   0: i18n._("{plural, ...}")
 }
})

// instead of single i18n statement:
i18n._("Hello {plural, ...}")

However, this hasn't happened often for users, or at least they don't open tickets for that.

But that is the case for experimental deps extractor. Look to the code generated by esbuild:

Esbuild bundle
// src/pages/index.page.tsx
import { Plural as Plural2, t as t2, Trans as Trans3 } from "@lingui/macro";
import path from "path";
import Head from "next/head";

// src/components/AboutText.tsx
import { Trans } from "@lingui/macro";
function AboutText() {
  return <p>
    <Trans>Hello, world</Trans>
    <br />
    <Trans id="message.next-explanation">Next.js is an open-source React front-end development web framework that enables functionality such as server-side rendering and generating static websites for React based web applications. It is a production-ready framework that allows developers to quickly create static and dynamic JAMstack websites and is used widely by many large companies.</Trans>
  </p>;
}

// src/components/Developers.tsx
import { useState } from "react";
import { Trans as Trans2, Plural } from "@lingui/macro";
function Developers() {
  const [selected, setSelected] = useState("1");
  return <div>
    <p><Trans2>Plural Test: How many developers?</Trans2></p>
    <div style={{ display: "flex", justifyContent: "space-evenly" }}>
      <select
        value={selected}
        onChange={(evt) => setSelected(evt.target.value)}
      >
        <option value="1">1</option>
        <option value="2">2</option>
      </select>
      <p><Plural value={selected} one="Developer" other="Developers" /></p>
    </div>
  </div>;
}

// src/components/Switcher.tsx
import { useRouter } from "next/router";
import { useState as useState2 } from "react";
import { msg } from "@lingui/macro";
import { useLingui } from "@lingui/react";
var languages = {
  en: msg`English`,
  sr: msg`Serbian`,
  es: msg`Spanish`
};
function Switcher() {
  const router = useRouter();
  const { i18n: i18n2 } = useLingui();
  const [locale, setLocale] = useState2(
    router.locale.split("-")[0]
  );
  function handleChange(event) {
    const locale2 = event.target.value;
    setLocale(locale2);
    router.push(router.pathname, router.pathname, { locale: locale2 });
  }
  return <select value={locale} onChange={handleChange}>{Object.keys(languages).map((locale2) => {
    return <option value={locale2} key={locale2}>{i18n2._(languages[locale2])}</option>;
  })}</select>;
}

// src/pages/index.page.tsx
import styles from "../styles/Index.module.css";

// src/utils.ts
import { i18n } from "@lingui/core";
import { useRouter as useRouter2 } from "next/router";
import { useEffect, useState as useState3 } from "react";
async function loadCatalog(locale, pathname) {
  if (pathname === "_error") {
    return {};
  }
  const catalog = await import(`@lingui/loader!./locales/src/pages/${pathname}.page/${locale}.po`);
  return catalog.messages;
}

// src/pages/index.page.tsx
import { useLingui as useLingui2 } from "@lingui/react";
var getStaticProps = async (ctx) => {
  const fileName = __filename;
  const cwd = process.cwd();
  const { locale } = ctx;
  const pathname = path.relative(cwd, fileName).replace(".next/server/pages/", "").replace(".js", "");
  const translation = await loadCatalog(locale || "en", pathname);
  return {
    props: {
      translation
    }
  };
};
var Index = () => {
  useLingui2();
  return <div className={styles.container}>
    <Head>
      {
        /*
         The Next Head component is not being rendered in the React
         component tree and React Context is not being passed down to the components placed in the <Head>.
         That means we cannot use the <Trans> component here and instead have to use `t` macro.
        */
      }
      <title>{t2`Translation Demo`}</title>
      <link rel="icon" href="/favicon.ico" />
    </Head>
    <main className={styles.main}>
      <Switcher />
      <h1 className={styles.title}><Trans3>
        {"Welcome to "}
        <a href="https://nextjs.org">Next.js!</a>
      </Trans3></h1>
      <h2><Trans3>Plain text</Trans3></h2>
      <h2>{t2`Plain text`}</h2>
      <h2><Trans3>
        <a href="https://nextjs.org">Next.js</a>
        {" say hi."}
      </Trans3></h2>
      <h2><Trans3>
        {"Wonderful framework "}
        <a href="https://nextjs.org">Next.js</a>
        {" say hi."}
      </Trans3></h2>
      <h2><Trans3>
        {"Wonderful framework "}
        <a href="https://nextjs.org">Next.js</a>
        {" say hi. And "}
        <a href="https://nextjs.org">Next.js</a>
        {" say hi."}
      </Trans3></h2>
      <div className={styles.description}><AboutText /></div>
      <Developers />
      <div>
        <Plural2 value={1} one="# Person" other="# Persons" />
        <br />
        <Plural2 value={2} one="# Person" other="# Persons" />
      </div>
    </main>
  </div>;
};
var index_page_default = Index;
export {
  index_page_default as default,
  getStaticProps
};

Due to how bundler works, there might be multiple import statements in one file and usages of macro could be mixed and matched from different imports.

Solution

I decided to stop fighting with babel-macro-plugin and just write a good-old babel plugin using macro just as hook to run my plugin.

function macro({ state, babel, config }: MacroParams) {
    const plugin = linguiPlugin(babel)

    const { enter, exit } = plugin.visitor.Program as VisitNodeObject<
      any,
      Program
    >

    enter(state.file.path, state)
    state.file.path.traverse(plugin.visitor, state)
    exit(state.file.path, state)
  }

  return { keepImports: true }
}

Due to Visitor and DFS tree traversal we always start from the root node and go deeper processing every nested macro it might have. Also even if we forgot to handle some case, thanks to Visitor pattern this node could be picked up and processed by plugin even if didn't manually cover this case.

Additionally, plugin heavily relies on babel built-in methods to work with scopes/bindings, and instead of "naive" matching lingui's macros by it's name, now plugin uses referencesImport() which consider all scope bindings in the file. Let's look on example:

import { t as myT } from "@lingui/macro"; // <- already smart enought to understand renaming

myT`Hello`;

function MyFunction() {
  let myT;

  myT`Hello`; // <-- Oops, this inner scope has binding with the same name, but we not smart enought to understand this 
}

Now this is not the case and plugin correctly understand scopes and shadowing of bindings.

Clash of inserted variables and existing ones

Previous implementation was so dumb about scopes and bindings that it may broke the runtime code entirely in specific circumstances:

import { t } from "@lingui/macro";

function MyFunction() {
  let i18n;

  t`Hello`;
}
// BEFORE
import { i18n } from "@lingui/core";

function MyFunction() {
  let i18n;

  i18n._("Hello"); // <- Broken! since the i18n already exsits in current scope
}

Now the plugin creates a unique identifier for every inserted binding and transformed code looks like:

// AFTER
import { i18n as _i18n1 } from "@lingui/core";

function MyFunction() {
  let i18n;

  _i18n1._("Hello"); // <- Works! no naming collisions
}

What's next?

In next iterations, i'm going to make this plugin publically available and start promoting it as a recomended way to setup lingui. So users which have access to the babel config may not need to install an additional babel-macro-plugin dependency and could use this plugin directly.

The macro hook would still be available for that users which doesn't have access to babel config, such as CRA or gatsby but would be less promoted.

Currently, the plugin is private and not exposed publically from the package.

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
  • Examples 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 the necessary documentation (if appropriate)

Copy link

vercel bot commented Feb 28, 2024

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

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 5, 2024 4:26pm

Copy link

github-actions bot commented Feb 28, 2024

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.86 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 97.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 77.23%. Comparing base (dd43fb0) to head (30c0a04).
Report is 2 commits behind head on next.

Files Patch % Lines
packages/macro/src/macroJs.ts 97.36% 1 Missing and 2 partials ⚠️
packages/macro/src/macroJsx.ts 92.30% 1 Missing and 1 partial ⚠️
packages/macro/src/index.ts 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1867      +/-   ##
==========================================
+ Coverage   76.66%   77.23%   +0.56%     
==========================================
  Files          81       83       +2     
  Lines        2083     2126      +43     
  Branches      532      549      +17     
==========================================
+ Hits         1597     1642      +45     
+ Misses        375      374       -1     
+ Partials      111      110       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thekip thekip marked this pull request as ready for review March 1, 2024 09:33
@andrii-bodnar andrii-bodnar changed the base branch from main to next March 1, 2024 13:19
@thekip thekip force-pushed the refactor/babel-macro-to-plugin branch from b0ec5f1 to ffc5241 Compare March 1, 2024 14:06
@thekip
Copy link
Collaborator Author

thekip commented Mar 1, 2024

@andrii-bodnar rebased. PR is ready for review.

@thekip
Copy link
Collaborator Author

thekip commented Mar 5, 2024

Added a separate test case for #1797

@andrii-bodnar is there anything I can do to speed up reviewing process?

@andrii-bodnar
Copy link
Contributor

@thekip the extractor-experimental-1797 test case looks extremely redundant, it contains a lot of files that are probably not needed for this. Can we isolate this case into a minimal set of required files and code?

@thekip
Copy link
Collaborator Author

thekip commented Mar 5, 2024

the extractor-experimental-1797 test case looks extremely redundant, it contains a lot of files that are probably not needed for this. Can we isolate this case into a minimal set of required files and code?

Well, I cover every separate issue by its own test in macro / extractor. So yeah, it's probably redundant. From the other side, this is a real test, not synthetic, which potentially could catch problems after updating esbuild, or other dependencies.

@andrii-bodnar
Copy link
Contributor

Well, I cover every separate issue by its own test in macro / extractor. So yeah, it's probably redundant. From the other side, this is a real test, not synthetic, which potentially could catch problems after updating esbuild, or other dependencies.

Understand. As I can see, we already have some similar tests for the experimental extractor. Let's follow them and limit the new test to the minimum amount of code and files needed. It probably doesn't need so many strings to test, a language switcher, styles, utils, tsconfig, and package.json.

@thekip
Copy link
Collaborator Author

thekip commented Mar 5, 2024

they all needed unfortunately, package.json is needed to determine "externals", such as next/header, tsconfig might be deleted... The rest of files are needed to simulate real-use case when many files bundled and esbuild crawl them in specific order (so there are appears multiple macro imports).

Styles needed also nice to have, to check that esbuild don't try to parse them.

All this is fixtures, we don't compare these files. We compare only resulting catalogs.

@andrii-bodnar
Copy link
Contributor

I'm trying to avoid committing excessive code. Let's reduce this test to the minimum size needed - delete unnecessary files and make the rest files as small as possible (we probably don't need 14 messages, 50+ lines of styles, or a 100-line component for the test).

@thekip
Copy link
Collaborator Author

thekip commented Mar 5, 2024

i don't have time to checking how changes in the imports / files structure after "cleaning" will affect bundled output of esbuild and will it still reproduce the issue or not. I reverted it.

@andrii-bodnar
Copy link
Contributor

@thekip thank you!

@andrii-bodnar andrii-bodnar merged commit 9b9e4a4 into lingui:next Mar 6, 2024
16 checks passed
andrii-bodnar pushed a commit that referenced this pull request Mar 6, 2024
@thekip thekip deleted the refactor/babel-macro-to-plugin branch March 6, 2024 08:35
@andrii-bodnar
Copy link
Contributor

@thekip are you planning to commit anything soon? I would like to make a new release if not

@thekip
Copy link
Collaborator Author

thekip commented Mar 6, 2024

@andrii-bodnar no you can go ahead. Make it as next as well, since change is a quite big, i want to test it on real projects before

@andrii-bodnar
Copy link
Contributor

@thekip
Copy link
Collaborator Author

thekip commented Mar 6, 2024

Tested on my project and on https://github.com/brave/ads-ui

works well, catalogs did not changed after extracting with new version and no errors during build.

@andrii-bodnar
Copy link
Contributor

Awesome!

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.

lingui extract-experimental bugs
2 participants