RFC-001 - Macros #318

tricoder42 · 2018-09-09T16:16:14Z

Implementation of RFC-001.

Resolves #298
Resolves #258
Resolves #197

Work in progress:

fix extracting messages from JSX macros
update webpack/react example
documentation: API reference for macros
documentation: React tutorial

In next batch:

core: i18n(...) API as a shortcut for i18n._(...)
documentation: JavaScript tutorial
macros: allow used them as standalone plugins with implicit imports
docs: Migration guide from plugins to macros (plugins still supported until v3)
plugins/presets: deprecate packages

codecov · 2018-09-09T22:00:50Z

Codecov Report

Merging #318 into master will increase coverage by 0.34%.
The diff coverage is 98.89%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   95.52%   95.86%   +0.34%     
==========================================
  Files          43       49       +6     
  Lines        1140     1379     +239     
==========================================
+ Hits         1089     1322     +233     
- Misses         51       57       +6

Impacted Files	Coverage Δ
packages/cli/src/api/extractors/babel.js	`22.22% <ø> (ø)`	⬆️
packages/macro/index.js	`0% <0%> (ø)`
...es/babel-plugin-transform-react/src/transformer.js	`100% <100%> (ø)`
packages/macro/src/index.js	`100% <100%> (ø)`
packages/cli/src/lingui-extract.js	`69.23% <100%> (+0.48%)`	⬆️
...ackages/babel-plugin-extract-messages/src/index.js	`99.1% <100%> (+0.12%)`	⬆️
packages/babel-plugin-transform-react/src/index.js	`100% <100%> (ø)`	⬆️
packages/babel-plugin-transform-react/index.js	`100% <100%> (ø)`
packages/cli/src/lingui-init.js	`79.16% <100%> (+0.9%)`	⬆️
packages/babel-plugin-transform-js/src/index.js	`100% <100%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce0ad4b...bab02db. Read the comment docs.

danielkcz · 2018-09-10T07:42:18Z

examples/webpack-react-macros/src/Usecases/ElementAttributes.js

+            <a
+              className="expression"
+              href="/article"
+              title={i18n._(t`Full content of ${articleName}`)}


Is it really an improvement considering how it was looking before? :) Was the previous approach working? I am not sure, haven't used that. Also, I think that in gitter we were talking about a variant like that which would be much nicer imo.

i18n(t`Full content of ${articleName}`)

You mean i18n.t over i18n._(t? It's necessary to move to macros... I don't think there's a way to have a object property to be a macro.

Other way would be convert it to i18n._ directly, but for that we would need i18n in scope, which wouldn't be used otherwise and linter would complain.

This approach is very flexible, but the only drawback is how it looks :/ But you really get used to it. I wrote vanilla-js example and it was so great.

i18n(t is planned, I just have to figure out how to create callable object. I already started working on it but then figure out this is not the core of the problem and can be done later. Both i18n._(..) and i18n(...) will be supported anyway until v3.

And yeah, the previous approach had several problems, especially when plugins were misconfigured. It doesn't allow you to use this.props.i18n.t, because only i18n.t worked. You couldn't use it with i18n.use method, which switches language locally (i18n.use('cs')._(t...)). And also for lazy messages you had to use different API, i18nMark. Now there's the same API for all messages, lazy/immediate translations, with/without IDs.

As I said, I like everything about it except how it looks... but I believe it won't be such a problem once we have i18n(t...) API.

Well, a "callable object" is just a function with additional properties/methods attached directly to it. There is nothing wrong about that in my opinion and it will make it much easier to use and look at :)

Cool! I believe there's nothing wrong with it as well, but it felt a bit hackish :) I'm halfway through with implementation, which I saved in different branch. I'll finalize it soon.

tricoder42 · 2018-09-10T08:07:38Z

@FredyC I've added checklist to PR description, so it's clear what's missing. I would like to release an early version asap, so I can play with it and get some feedback.

danielkcz · 2018-09-10T14:39:18Z

@tricoder42 Right ... I am a bit preoccupied for the next couple of days, but I will try to have a look occasionally.

tricoder42 · 2018-09-10T16:06:28Z

@FredyC Making i18n callable is a bit tricky. I've found few references on SO:

https://stackoverflow.com/questions/12656079/constructor-for-callable-object-in-javascript
https://stackoverflow.com/questions/19335983/can-you-make-an-object-callable

After fiddling a bit, I come up with this solution:

import { setupI18n as legacySetupI18n, I18n as legacyI18n } from "@lingui/core"

export function setupI18n(params = {}) {
  const i18n = legacySetupI18n(params)

  function _() {
    return i18n._.apply(i18n, arguments)
  }

  // this is unsupported by flow
  _.__proto__ = i18n

  return _
}

The only problem is that the line _.__proto__ is unsupported by flow, so I can't add type annotations to this file, otherwise it would break type checking.

I would appreciate any help here :)

danielkcz · 2018-09-10T16:50:27Z

I don't think you need to care about prototype at all. Instead of having class I18n you would have something like this. I haven't tested it, but I think it won't be far from the truth. Regarding Flow, this should help.

function setupI18n(params = {}) {
  let _language: string
  let _locales: ?Locales
  // all other private properties

  // this is instead of a current `constructor`
  function i18n() {
    if (process.env.NODE_ENV !== "production") {
      this.t = t
      this.select = select
      this.plural = plural(this)
      this.selectOrdinal = selectOrdinal(this)
    }
  }

  Object.assign(i18n, {
    get availableLanguages(): Array<string> {
      return Object.keys(this._catalogs)
    }

    get language(): string {
      return this._language
    }

    get locales(): ?Locales {
      return this._locales
    }
    // etc...
  })

  return i18n
}

tricoder42 · 2018-09-10T19:57:45Z

Yeah, but you're not creating a callable object. What you have is a pattern to create an object with some method on contructor (you should assign properties to i18n.prototype, btw).

What we need is a function and somehow assign a i18n object to function's prototype. That way the function is callable, but "inherits" properties/methods from i18n object.

I've just moved proto assignment to external file to make Flow happy and added flow types in recent commit: b8691d3. It works. I tried few variation, but this is the only one which works. Not sure if there's a better way, but suggestions welcome :)

tricoder42 · 2018-09-10T20:24:00Z

Alright, I reverted this commit and moved it to https://github.com/lingui/js-lingui/tree/rfc/001-macros-callable-i18n branch... It's more complex than expected and it need to think through.

EDIT: integration tests failed, because language switching doesn't work. Not sure if it's caused by mixing ES6 classes with classical prototypal inheritance, or by setting prototype of a Function.

tricoder42 · 2018-09-10T20:49:21Z

Let's ship the pig and see how it works 🤞

danielkcz · 2018-09-11T05:58:22Z

Yeah, but you're not creating a callable object. What you have is a pattern to create an object with some method on contructor (you should assign properties to i18n.prototype, btw).

What we need is a function and somehow assign a i18n object to function's prototype. That way the function is callable, but "inherits" properties/methods from i18n object.

I don't think you are right about this. Even if you look at the Flow doc I've linked, they are creating callable object like this. It's the exact same approach I've drafted above.

function add(x, y) {
  return x + y; 
}
add.bar = "hello world";

Yes, every function can be a constructor, you can call it with new which essentially means it creates a new instance of an object based on a prototype. As long as you don't use new, you have just an function with properties/methods that can be called and thanks to the closure you have access to private variables. Function is really just an object with system property [[Call]].

I don't think I am going to use this "pig" just yet, because it's too ugly to have in the code :)

tricoder42 · 2018-09-11T08:40:51Z

I have to review it again. These things just give me a headache.

And common, it's not that bad :) i18n(t'Hello') vs i18n._(t'Hello') is just a small difference :) To be honest, I would love to remove the parenthesis completely, but I don't know how to combine macro and i18n object... This is still suboptimal, the previous version looked better (i18n.t'Hello'), it was just buggy :/

danielkcz · 2018-09-11T09:09:58Z

I guess this is a limitation of macros, they are nice and all, but with reliability on actual imports, it makes it much verbose. But would it be too bad to have a separate optional babel plugin that could convert current i18n.t'Hello' to an actual i18n._(t'Hello') before macros would work on that?

tricoder42 · 2018-09-11T09:19:56Z

We'll see. I would like to have just a one way how to do these things, otherwise it's gonna be harder to maintain

tricoder42 added 5 commits September 9, 2018 10:16

docs: new version of rfc 001

cc944a7

docs: add message descriptions to rfc

3e66923

feat: add macros

8a21e21

test: add example with macros

d16135a

test: add vanillaJS example

72ecce1

tricoder42 self-assigned this Sep 9, 2018

tricoder42 added 6 commits September 9, 2018 19:47

test: add example of lazy translations

3613c9b

feat: add jsx macros

401569f

test: pass plugin options if exists

66568cd

test: run vanilla-js integration test suit separately

1d84809

test: fix typo

e53ecd9

test: install yalc packages after yarn

921d5aa

test: make lingui executable

4897e64

danielkcz reviewed Sep 10, 2018

View reviewed changes

tricoder42 added 4 commits September 10, 2018 11:14

fix: keep imports for extracting messages

cf62467

test: update webpack react example to use macros

0e08f9b

test: remove obsolete example and make macros/babel 7 the default one

7a4da55

test: run webpack example test suit separately

a6ddbde

tricoder42 added 2 commits September 10, 2018 17:06

docs: update docs to use macros instead of plugins

316cc0e

feat: update init command to install macros instead of plugins

7562f39

tricoder42 force-pushed the rfc/001-macros branch from 35f34fe to b8691d3 Compare September 10, 2018 19:54

tricoder42 force-pushed the rfc/001-macros branch from b8691d3 to 7562f39 Compare September 10, 2018 20:22

docs: update READMEs

bab02db

tricoder42 merged commit eb3540c into master Sep 10, 2018

tricoder42 deleted the rfc/001-macros branch September 10, 2018 20:47

danielkcz mentioned this pull request Oct 3, 2018

[Epic] Road to LinguiJS v3 #334

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC-001 - Macros #318

RFC-001 - Macros #318

tricoder42 commented Sep 9, 2018 •

edited

Loading

codecov bot commented Sep 9, 2018 •

edited

Loading

danielkcz Sep 10, 2018 •

edited

Loading

tricoder42 Sep 10, 2018

tricoder42 Sep 10, 2018

danielkcz Sep 10, 2018

tricoder42 Sep 10, 2018

tricoder42 commented Sep 10, 2018

danielkcz commented Sep 10, 2018

tricoder42 commented Sep 10, 2018

danielkcz commented Sep 10, 2018

tricoder42 commented Sep 10, 2018

tricoder42 commented Sep 10, 2018 •

edited

Loading

tricoder42 commented Sep 10, 2018 •

edited

Loading

danielkcz commented Sep 11, 2018 •

edited

Loading

tricoder42 commented Sep 11, 2018

danielkcz commented Sep 11, 2018 •

edited

Loading

tricoder42 commented Sep 11, 2018

RFC-001 - Macros #318

RFC-001 - Macros #318

Conversation

tricoder42 commented Sep 9, 2018 • edited Loading

codecov bot commented Sep 9, 2018 • edited Loading

Codecov Report

danielkcz Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

tricoder42 Sep 10, 2018

Choose a reason for hiding this comment

tricoder42 Sep 10, 2018

Choose a reason for hiding this comment

danielkcz Sep 10, 2018

Choose a reason for hiding this comment

tricoder42 Sep 10, 2018

Choose a reason for hiding this comment

tricoder42 commented Sep 10, 2018

danielkcz commented Sep 10, 2018

tricoder42 commented Sep 10, 2018

danielkcz commented Sep 10, 2018

tricoder42 commented Sep 10, 2018

tricoder42 commented Sep 10, 2018 • edited Loading

tricoder42 commented Sep 10, 2018 • edited Loading

danielkcz commented Sep 11, 2018 • edited Loading

tricoder42 commented Sep 11, 2018

danielkcz commented Sep 11, 2018 • edited Loading

tricoder42 commented Sep 11, 2018

tricoder42 commented Sep 9, 2018 •

edited

Loading

codecov bot commented Sep 9, 2018 •

edited

Loading

danielkcz Sep 10, 2018 •

edited

Loading

tricoder42 commented Sep 10, 2018 •

edited

Loading

tricoder42 commented Sep 10, 2018 •

edited

Loading

danielkcz commented Sep 11, 2018 •

edited

Loading

danielkcz commented Sep 11, 2018 •

edited

Loading