redux-polyglot icon indicating copy to clipboard operation
redux-polyglot copied to clipboard

change translate() parameter

Open guillaumearm opened this issue 9 years ago • 9 comments

call translate(DummyComponent) should be forbidden.

should be call like this :

  • translate()(Dummy)
  • translate('')(Dummy)
  • translate('', Dummy)

guillaumearm avatar Oct 12 '16 08:10 guillaumearm

As we discussed, I'd personally prefer that translate can be called either :

  • translate(MyComponent)
  • translate('some.polyglot.scope', MyComponent)
  • translate('some.polyglot.scope')(MyComponent)

Why would you want to forbid the first one ?

JalilArfaoui avatar Oct 13 '16 05:10 JalilArfaoui

because it's more simple to use and to remind.

  • i think it's better when arguments orders is always the same.
  • so, signature will be: translate :: polyglotScope -> Component -> TranslatedComponent
  • bonus: it is easier to migrate from react-polyglot

guillaumearm avatar Oct 13 '16 09:10 guillaumearm

Mmm .. why not. And do we still consider this usage :

translate({ polyglotScope: 'some.polyglot.scope' })(MyComponent)

or, in other words :

translate(options)(MyComponent)

?

It seems more complicated, but it enables us to support more option if the future, without changing signature.

JalilArfaoui avatar Oct 18 '16 19:10 JalilArfaoui

One more thing ... maybe we could choose between

  • translate('scope', MyComponent)
  • translate('scope')(MyComponent)

If we want to maintain both, and keep using curry, to handle this call :

translate()(MyComponent)

We will have to test first arg type in translate to know if it's option or component.

This is Because of the way curry works :

const translate = curry(
  (options, component) => {
    console.log('options: '+options);
    console.log('component: '+component);
  }
);

translate()('component')('props');

output :

options: component
component: props

JalilArfaoui avatar Oct 18 '16 21:10 JalilArfaoui

let's go for :

  • translate({ defaultPolyglotScope: 'scope' })(MyComponent)
  • translate()(MyComponent)

guillaumearm avatar Oct 20 '16 16:10 guillaumearm

or translate({ polyglotScope: 'scope' })(MyComponent) as you wish.

guillaumearm avatar Oct 29 '16 23:10 guillaumearm

At the moment translate()(MyComponent) doesn't work which is quite strange as I use currying as it seems to be what the community is doing (need to do translate('')(MyComponent)).

Would you accept a PR to fix this use-case and land it on the current version? As I can see, the goal is later to move the react specific functionality to react-redux-polyglot correct?

satazor avatar Dec 06 '16 14:12 satazor

Yes, it is a known problem. We have to uncurry translate() so that it always returns a function. Only drawback is that translate(MyComponent) won't work anymore but it won't in the future anyways so I guess it's not really a problem to do it now. PR welcome :-)

As to the future of react-redux-polyglot package, we are discussing to have it as another npm module but in the same repo.

JalilArfaoui avatar Dec 06 '16 16:12 JalilArfaoui

I've started to refactor translate : https://github.com/Tiqa/redux-polyglot/tree/feat/multi_module_refacto WIP, I'll continue tomorrow

JalilArfaoui avatar Dec 07 '16 00:12 JalilArfaoui