Add i18n data store #47589

sarayourfriend · 2020-11-19T13:50:56Z

Changes proposed in this Pull Request

Adds a new i18n data store to data-stores with a single selector/resolver pair for fetching localized language names.

Testing instructions

Ensure unit tests and build passes.

matticbot · 2020-11-19T13:50:57Z

Test live: https://calypso.live/?branch=add/i18n-data-store

matticbot · 2020-11-19T14:08:49Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

tyxla

Thanks for splitting the giant PR, @saramarcondes, that's really helpful 🙇‍♂️

The code looks great! Since it's not used anywhere, I think we can ship it as-is already. 🚢

I've left some comments and observations, but I don't think there's anything blocking and most of the items can be followed up with in future PRs.

Let me know what you think.

tyxla · 2020-11-20T12:59:08Z

packages/data-stores/src/i18n/resolvers.ts

+		// Forcefully add `_locale` here because the `data` parameter will re-write it as
+		// just `locale` (no underscore). Context here:
+		// https://github.com/Automattic/wp-calypso/pull/46328#discussion_r515674976
+		url: `https://public-api.wordpress.com/wpcom/v2/i18n/language-names?_locale=${ locale }`,


I know we're doing this for other stores already, but I think we could iterate on it: could we use createRootURLMiddleware to set the root URL instead of having to specify it as a path?

This would also allow us to specify the namespace by using the namespaceAndEndpointMiddleware middleware inside our root URL middleware.

This could come in handy for other data stores that work directly with the wp.com REST API.

Would the rootURL be https://public-api.wordpress.com/wpcom? Perhaps we should open an issue for this refactor given it will touch other stores as well?

I'm assuming the root URL would be https://public-api.wordpress.com/ and wpcom/v2 would actually be the namespace. If you inspect Calypso, you'll see plenty of requests to the same root URL, but for different namespaces - for example rest/v1.1 or rest/v1.3.

But yep, an issue makes sense 👍 . By all means, I don't think it should block this PR anyway.

packages/data-stores/src/i18n/index.ts

tyxla · 2020-11-20T13:07:20Z

packages/data-stores/src/i18n/resolvers.ts

+
+export function* getLocalizedLanguageNames( locale: string ) {
+	const localizedLanguageNames = yield apiFetch( {
+		// Forcefully add `_locale` here because the `data` parameter will re-write it as


I wasn't sure why it rewrites it to just locale (no underscore). Could you elaborate?

Furthermore, could we overwrite the default userLocaleMiddleware middleware with our own one that would work with _locale properly?

I wasn't sure why it rewrites it to just locale (no underscore). Could you elaborate?

I think I was confused. I can't reproduce this anymore.

Furthermore, could we overwrite the default userLocaleMiddleware middleware with our own one that would work with _locale properly?

You tell me! I don't know anything about apiFetch middleware other than that they exist. I've taken a quick look and I'm not sure what to suggest how to start. I think we also need the argument lest the resolver only be called the first time even if the locale changes (it's my understanding that the resolver will only be called the first time the selector is called with any given argument set?). Given that, I think it's a requirement that the locale be parameterized to the resolver.

I think I was confused. I can't reproduce this anymore.

👍 Yeah, can't reproduce it either, FWIW.

You tell me! I don't know anything about apiFetch middleware other than that they exist. I've taken a quick look and I'm not sure what to suggest how to start.

In general this would boil down to creating our own middleware just like here and doing a apiFetch.use( ourUserLocaleMiddleware ) before executing the actual apiFetch().

Again, though, this is not a blocker, and definitely something we can try later if you're interested.

packages/data-stores/src/i18n/selectors.ts

packages/data-stores/src/i18n/test/reducer.js

tyxla · 2020-11-20T13:11:54Z

packages/data-stores/src/i18n/test/resolvers.js

+			const iter = getLocalizedLanguageNames( 'en-us' );
+
+			expect( iter.next().value ).toEqual( {
+				type: 'API_FETCH',


This appears to be testing against the internals of the apiFetch data control. Is there away to abstract it, like for example, to use apiFetch() to generate this action?

I don't think so, unless we want to replicate the entire call to apiFetch (which isn't so much a test as a behavior lock-in). What do you think? Personally I find these tests weak to begin with.

The thing is that in the code we're using apiFetch() but in the test, we're testing against API_FETCH action type. I'd assume that the code we're testing and the tests would match; while right now, the test is testing against an internal of the external apiFetch() function.

In theory, if that implementation changes over time, our code will continue working, but the test will fail, and that's not what I'd expect to happen. So I tend to think on the contrary: these tests are actually more flexible and sustainable to future changes. But I'd appreciate it if you could clarify why you consider such tests weak, maybe I'm misunderstanding something.

Furthermore, isn't using the apiFetch data control here just fine? It has the same signature as the action we're creating in this test?

tyxla · 2020-11-20T13:14:37Z

packages/data-stores/src/i18n/test/resolvers.js

+
+			const localizedLanguageNames = Symbol( 'localizedLanguageNames' );
+
+			expect( iter.next( localizedLanguageNames ).value ).toEqual(


Should we add one last test case here to ensure that after this one, in the last fulfilment done is true?

Wouldn't that just lock the function down needlessly? For the purposes of this test, what does it care if something additional is added or done?

In fact, these tests are already so flimsy. If someone decides to add a middle step between API Fetch and setting the localized language names, then they'll need to update the test to reflect that new ordering... simply by adding the exact same call they made in the code to the test itself. That seems silly to me. I'm not sure these tests are useful at all.

FWIW, there are a bunch of those tests in Gutenberg for where action generators are being used. I find them pretty useful, to be honest, for two reasons:

Generators aren't just functions, so sometimes understanding them can be difficult from a first read. Reading the tests, though, makes that way easier.

Generators can end up being complex sometimes, and verifying that their execution flow produces the same results and yields the same output is crucial for ensuring we're not breaking them as we iterate.

Anyway, as pointed out before, I don't consider this a blocker for the PR and I'm happy to accept whatever you prefer to go ahead with here.

tyxla · 2020-11-20T13:14:56Z

packages/data-stores/src/i18n/test/resolvers.js

+		it( 'should set the localized language names', () => {
+			const iter = getLocalizedLanguageNames( 'en-us' );
+
+			expect( iter.next().value ).toEqual( {


In addition to checking the value, should we also be checking the done on every round?

tyxla

This LGTM, let's ship it 🚢

sarayourfriend requested review from yuliyan and tyxla November 19, 2020 13:50

sarayourfriend self-assigned this Nov 19, 2020

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 19, 2020

tyxla approved these changes Nov 20, 2020

View reviewed changes

tyxla added Data i18n Packages labels Nov 20, 2020

sarayourfriend changed the base branch from master to trunk November 20, 2020 16:10

sarayourfriend force-pushed the add/i18n-data-store branch from e471ef2 to fe62ae2 Compare November 20, 2020 23:28

sarayourfriend mentioned this pull request Nov 23, 2020

data-stores: Use createRootURLMiddleware to improve data store apiFetch request building #47675

Closed

sarayourfriend added 3 commits November 23, 2020 08:47

Add i18n data store

c362b66

Use URL utilities to add query param and expand tests

11758d4

Further refine tests

008ae1e

sarayourfriend force-pushed the add/i18n-data-store branch from 319d851 to 008ae1e Compare November 23, 2020 16:47

Resolve export type issues

0cdc02d

sarayourfriend requested a review from tyxla November 24, 2020 18:53

tyxla approved these changes Nov 25, 2020

View reviewed changes

sarayourfriend merged commit c998d4c into trunk Nov 25, 2020

sarayourfriend deleted the add/i18n-data-store branch November 25, 2020 17:45

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add i18n data store #47589

Add i18n data store #47589

sarayourfriend commented Nov 19, 2020

matticbot commented Nov 19, 2020

matticbot commented Nov 19, 2020

tyxla left a comment

tyxla Nov 20, 2020

sarayourfriend Nov 20, 2020

tyxla Nov 23, 2020

sarayourfriend Nov 23, 2020

tyxla Nov 20, 2020

sarayourfriend Nov 20, 2020

tyxla Nov 23, 2020

tyxla Nov 20, 2020

sarayourfriend Nov 20, 2020

tyxla Nov 23, 2020

tyxla Nov 20, 2020

sarayourfriend Nov 20, 2020

tyxla Nov 23, 2020

tyxla Nov 20, 2020

tyxla left a comment


		const localizedLanguageNames = Symbol( 'localizedLanguageNames' );

		expect( iter.next( localizedLanguageNames ).value ).toEqual(

Add i18n data store #47589

Add i18n data store #47589

Conversation

sarayourfriend commented Nov 19, 2020

Changes proposed in this Pull Request

Testing instructions

matticbot commented Nov 19, 2020

matticbot commented Nov 19, 2020

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment