Skip to content
This repository has been archived by the owner. It is now read-only.

feat(mc): #2813 Add react pre-rendering #2816

Closed
wants to merge 1 commit into from
Closed

Conversation

@k88hudson
Copy link
Member

@k88hudson k88hudson commented Jul 1, 2017

Fix #2813. This patch adds react prerendering to html, in order to improve the perceived intial load time.

There are a few remaining issues:

  • The prerendering assumes en-US strings and ltr
  • The prerendering assumes top sites and search are pref'd on. The case where a user has either pref'd off needs to be handled.
@k88hudson k88hudson force-pushed the k88hudson:gh2813 branch from dc9e44f to a7205e2 Jul 1, 2017
@k88hudson k88hudson force-pushed the k88hudson:gh2813 branch from a7205e2 to bd5789f Jul 1, 2017
@coveralls
Copy link

@coveralls coveralls commented Jul 1, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling bd5789f on k88hudson:gh2813 into b8f0c88 on mozilla:master.

@coveralls
Copy link

@coveralls coveralls commented Jul 1, 2017

Coverage Status

Coverage remained the same at 87.576% when pulling bd5789f on k88hudson:gh2813 into b8f0c88 on mozilla:master.

@k88hudson
Copy link
Member Author

@k88hudson k88hudson commented Jul 10, 2017

@sarracini want to take a look?

@k88hudson k88hudson requested review from dmose and removed request for sarracini Jul 17, 2017
@k88hudson k88hudson assigned dmose and unassigned sarracini Jul 17, 2017
@k88hudson
Copy link
Member Author

@k88hudson k88hudson commented Jul 17, 2017

@dmose, maybe a second pair of eyes wouldn't hurt?

@sarracini
Copy link
Contributor

@sarracini sarracini commented Jul 17, 2017

I don't know if we should land this until we figure out the two issues you mentioned, they seem sort of like big deals

@dmose
Copy link
Member

@dmose dmose commented Jul 17, 2017

It does seem hard to imagine that we could ship without those things handled, for some definition of handled. It seems like the question of the day is whether that work should be prioritized over the other stuff in "das Boot".

For the l10n stuff, the ideal case would be have pre-rendered pages for every language that we have a locale for, and just select the one to use at package-time, install-time, or (worst-case) run-time. Doing a simple version that just packages everything for all the locales sounds straightforward; you could ping mossop about whether he'd accept that.

Another possibly simple way forward would be to ship an English pre-rendered page, and a non-prerendered page, so at least the en-* case would get better perf.

I seem to recall us speculating about already knowing the maximum block-sizes of where the text is gonna be, so we could just pre-render without text and have text rendered in at load time.

What would handling the pref work likely involve?

@dmose dmose removed their assignment Jul 21, 2017
@k88hudson
Copy link
Member Author

@k88hudson k88hudson commented Jul 31, 2017

Closing this in favour of refactoring at a later date

@k88hudson k88hudson closed this Jul 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants