Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When streaming API is disconnected, poll home/notifications #2776

Merged
merged 2 commits into from May 4, 2017

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented May 3, 2017

  • Display slightly different empty home timeline message if user is following others (see Empty home timeline after period of inactivity #2619)
  • Cull notifications to 20 items when over 40 get added in real-time, just like happens in other columns
  • Run manage:translations

image

Display slightly different empty home timeline message if user is following others
Cull notifications to 20 items when over 40 get added in real-time
Run manage:translations
@Gargron Gargron requested a review from ykzts May 3, 2017 23:40

if (hasFollows) {
emptyMessage = <FormattedMessage id='empty_column.home.inactivity' defaultMessage="Your home feed is empty. If you have been inactive for a while, it will be regenerated for you soon." />
}
Copy link
Sponsor Member

@ykzts ykzts May 4, 2017

Choose a reason for hiding this comment

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

I think it would be better to create methods like renderEmptyMessage and renderEmptyMessageHasFollows. Execution of React.createElement takes some time.

const emptyMessage = hasFollows ? this.renderEmptyMessage() : this.renderEmptyMessageHasFollows();

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless that is moved out of the class I don't see how it would improve performance, since renderEmptyMessage() would also execute React.createElement live?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! I see what you mean. This could also be an if/else then. Fair enough

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I agree.

let emptyMessage;

if (hasFollws) {
  emptyMessage = ...;
} else {
  emptyMessage = ...;
}

It is also good to write like the above.

"notifications.column_settings.follow": "متابعُون جُدُد :",
"notifications.column_settings.favourite": "المُفَضَّلة :",
"notifications.column_settings.mention": "الإشارات :",
"notifications.column_settings.reblog": "الترقيّات:",
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all a result of yarn run manage:translations, which sorts JSON locale files alphabetically, etc.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yes. This is a comment for when other people saw it.

@nightpool
Copy link
Member

nightpool commented May 4, 2017

Is it common practice to add the english translations to the specific locale files? Can't we use a default message instead, to avoid polluting the files?

(or is it good because it calls out what needs to be changed, I guess?)

@szbalint
Copy link

szbalint commented May 4, 2017

Do I understand it right that the fallback would happen silently from the streaming api to polling?

If so, would it make sense to configuration-gate this? Otherwise it'd be quite hard to notice if the streaming api is unexpectedly down as opposed to running on a hosting environment where it's not supported.

@Gargron
Copy link
Member Author

Gargron commented May 4, 2017

@szbalint You would see WS connection failing in console logs. What kind of configuration-gate are you thinking of?

@szbalint
Copy link

szbalint commented May 4, 2017

@Gargron I mean for this feature from a maintainability/ops perspective:

  • an instance either runs the WS streaming service in which case it's better if that fails visibly when the service is down, as falling back to notifications would mask any issues until someone checks the console
  • or an instance can't/won't run WS based streaming in which case there shouldn't be connection attempts to WS/defined endpoint in the configuration file.

It looks to me that those two usecases aren't overlapping so it'd make sense for me if instance admins can decide in the .env.production config or somewhere else whether they run the streaming service or not and if not that's when they'd get polling as a fallback.

@Gargron Gargron merged commit eddb95b into master May 4, 2017
@Gargron Gargron deleted the feature-poll-fallback branch May 4, 2017 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants