Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

feat(systemaddon): Get Pocket feed for the appropriate locale. Closes… #2991

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

csadilek
Copy link
Collaborator

@csadilek csadilek commented Jul 27, 2017

Fix #2974.

Currently requesting other locales than en-* will return the exact same content, so there's no harm in putting this in now. Once other language feeds are ready they should just work (we won't need any client-side changes then).

Relying on accept-lang is possible in a future endpoint version, but in the meantime let's use the locale_lang request parameter. This is also consistent with A-S lang settings, as there we're not relying on accept-lang, but on general.useragent.locale.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.407% when pulling be2da3c on csadilek:2974 into 580602e on mozilla:master.

Copy link
Member

@Mardak Mardak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch for LOCALE_UPDATED to get locale changes to update the endpoint. Also reuse the existing state instead of Services.locale.getRequestedLocale()

this.stories_endpoint = this._produceUrlWithApiKey(options.stories_endpoint, apiKey);
this.topics_endpoint = this._produceUrlWithApiKey(options.topics_endpoint, apiKey);
const locale = Services.locale.getRequestedLocale();
this.stories_endpoint = this._produceFinalEndpointUrl(options.stories_endpoint, apiKey, locale);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that fetch* is called later in init, but does this actually need to happen during init? We ran into an issue where reftests were failing because network connections were being made aggressively (and denied).

But even ignoring the network connection issue (already fixed for now), instead of using Services.locale.getRequestedLocale, onAction should watch for LOCALE_UPDATED and have a helper method set the endpoint and maybe fetch. The helper would be called from LOCALE_UPDATED and init and can get the locale from this.store.getState().App.locale

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe calling fetch* in init is best. We want this to happen as soon as possible to increase the chance we have initial/fresh stories and topics fetched when a new tab is opened.

I will add the refactoring you requested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to fix the "later" than init network now, but I was thinking not that much long after init. It's just that anything done synchronously during the constructor and INIT counts against one of the talos performance metrics.

Copy link
Member

@Mardak Mardak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll go with this approach after some discussion to refactor with #2999 later

Also ignoring the issue of not watching for LOCALE_UPDATED for now as it's an uncommon issue. #3000

@Mardak Mardak merged commit b3834bb into mozilla:master Jul 27, 2017
@as-pine-proxy
Copy link
Collaborator

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants