search

Tiqa / redux-polyglot

Polyglot.js bindings for Redux
MIT License
58 stars 13 forks source link

perfomance: Create a new getP selector for each translate instance #89

Closed lneves12 closed 6 years ago

lneves12 commented 6 years ago

Problematic Atm there is a global getP selector for all the translate hoc components. As soon as a translate hoc is added to a page with custom options, for instance the option polyglotScope, it will mess with all the other existent translates as the reselect selector only keep cache of one element and we are passing these options as selector props.

Solution Create a new getP selector for each translate, so each instance keeps their own cache instead of conflicting with all the other translates.

Any other ideas of how to tackle this issue?

@satazor

guillaumearm commented 6 years ago

Hi @lneves12

Congratulations, you will be the 7th contributor on redux-polyglot project. Thanks for helping this library growing.

I thinks it's a good approach. selector factories is most of the time a good solution in this kind of situation.

Some steps before merging :

1. Tests

You need to write some tests to highlight this issue.

2. Discuss

We will wait for the opinion of @JalilArfaoui and @satazor. (@nemosupremo, @0xClpz and @jvincent42 are welcome too)

~For instance, maybe we need to talk about the name of the function. createGetP or makeGetP ?~

~3. Documentation~

~You need to write documentation about this. I know, the redux-polyglot documentation is not very pretty, but we need to keep it up to date.~

Edit: woops sorry I'm tired, createGetP already exists.

satazor commented 6 years ago

LGTM, I made the review with just one little thing regarding the naming.

This PR needs tests though.

lneves12 commented 6 years ago

Thanks for the fast review :)

I can add some tests and normalize the naming of the selector factories as soon as I have some time. Probably during tomorrow

lneves12 commented 6 years ago

Just added some tests for my changes and normalised the selector factories as @satazor suggested.

I ran my test before my changes and it went from red->green.

Let me know if you need anything else to be added to this pull request :)

satazor commented 6 years ago

I think this is good to be merged @guillaumearm

satazor commented 6 years ago

Bump @guillaumearm

guillaumearm commented 6 years ago

Sorry for the late, it was a difficult week for me. I will merge this PR and release a 0.6.3 version tonight.

lneves12 commented 6 years ago

Awesome. Thanks @guillaumearm