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

Small perf imps #26

Merged
merged 3 commits into from Sep 22, 2018

Conversation

Projects
None yet
2 participants
@denar90
Copy link
Contributor

denar90 commented Sep 15, 2018

Lets add more perf ❤️ for perf tooling 😍


tltr;

Loading app in prod mode users see unstyled content until bundle is loaded. On junky connections it look not really nice.
(I know, who will open progressive tooling on a $300 phone via 2/3G while this project for developers? but…).

Why is it happening?

  1. All content is rendered during build time. When page delivered to users it’s delivered unstyled
  2. It’s unstyled until bundle is loaded because CSS in JS - https://github.com/emotion-js/emotion is used and all styles coupled with js, so users will wait...

image

How to fix it?
Critical CSS might be the answer.

But…

  1. We should extract critical css using emotion during ssr, see docs
  2. Problem here is that preact-cli do aka ssr during build time under the hood, see here.
  3. To solve it, all pipeline has to be changed. We shouldn’t do prerender during build, but do it with real ssr. To do so, we have to add additional code for setting up server and deploy it on firebase. When true ssr will be available then extractCritical will be possible for the app.

Please, correct if I was mistaken during investigation ;)

Temporary solution is to hide body content until bundle loaded and then show all styled app.

Here the perf results:

image

Result - SI increased.

denar90 added some commits Sep 14, 2018

@denar90

This comment has been minimized.

Copy link
Contributor Author

denar90 commented Sep 16, 2018

Thank's to @developit's tip I've played with adding critical css during ssr and it works!!! - d691b37
Need polishing a bit and solving strange issue brought with emotion-server

bundle.f58a3.js:formatted:5792 Uncaught ReferenceError: process is not defined

But results are pretty good

image

@housseindjirdeh

This comment has been minimized.

Copy link
Collaborator

housseindjirdeh commented Sep 17, 2018

This is so so so amazing 😍

Everything you've said is spot on. I've been meaning to explore adding critical styles this week since the flash of unstyled content is a bit annoying, especially in slow connections I agree 😅

Thank you so much for considering a workaround to at least hide content until it gets styled. I was considering kicking off a thread to discuss the possibility of adding complete SSR for this since I wasn't sure preact-cli supported this, but I'm so glad to hear it's possible. Speed index improvements look absolutely amazing!

Again, thank you so much for this. Somebody noticed a similar issue using styled-components + preact here, but I don't know if they were extracting styles for the server in this case or even if this is remotely related 🤔

@denar90

This comment has been minimized.

Copy link
Contributor Author

denar90 commented Sep 17, 2018

Thanks 😊

I was trying to fix this problem, but looks like some related code for nodejs became part of a build

'use strict';

if (!process.version ||
    process.version.indexOf('v0.') === 0 ||
    process.version.indexOf('v1.') === 0 && process.version.indexOf('v1.8.') !== 0) {
  module.exports = { nextTick: nextTick };
} else {
  module.exports = process
}

function nextTick(fn, arg1, arg2, arg3) {
  if (typeof fn !== 'function') {
    throw new TypeError('"callback" argument must be a function');
  }
  var len = arguments.length;
  var args, i;
  switch (len) {
  case 0:
  case 1:
    return process.nextTick(fn);
  case 2:
    return process.nextTick(function afterTickOne() {
      fn.call(null, arg1);
    });
  case 3:
    return process.nextTick(function afterTickTwo() {
      fn.call(null, arg1, arg2);
    });
  case 4:
    return process.nextTick(function afterTickThree() {
      fn.call(null, arg1, arg2, arg3);
    });
  default:
    args = new Array(len - 1);
    i = 0;
    while (i < args.length) {
      args[i++] = arguments[i];
    }
    return process.nextTick(function afterTick() {
      fn.apply(null, args);
    });
  }
}




// WEBPACK FOOTER //
// ../node_modules/process-nextick-args/index.js

And it's present only when extractCritical (import { extractCritical } from 'emotion-server';) is imported.
I assume it happens because create-emotion-server exports also methods like renderStylesToNodeStream which works with node stream which requires process-nextick-args and that's why error appears.

I did workaround and cheated :) copy/pasting just function createExtractCritical and it works, partially.
But it will be great not to have realted stuff in bundle by default just via

import { extractCritical } from 'emotion-server';

cc @developit @mitchellhamilton

@housseindjirdeh

This comment has been minimized.

Copy link
Collaborator

housseindjirdeh commented Sep 18, 2018

Cheers, I appreciate you considering adding the method in yourself 🚀

Agreed, would be nice if we can pinpoint why this issue is happening and hopefully have it resolved upstream. But if that doesn't happen soon enough, I'm okay with this workaround for the meantime :)

Thanks again matey, we owe you one 🙏

@denar90

This comment has been minimized.

Copy link
Contributor Author

denar90 commented Sep 21, 2018

I've updated PR.
It took some time to handle ssr and client mode, moreover ssr is not run for dev build 😓 but now everything works good 😄
Critical css extracted 🎉 FOUC - eliminated

@denar90

This comment has been minimized.

Copy link
Contributor Author

denar90 commented Sep 21, 2018

Cheers, I appreciate you considering adding the method in yourself 🚀

Agreed, would be nice if we can pinpoint why this issue is happening and hopefully have it resolved upstream. But if that doesn't happen soon enough, I'm okay with this workaround for the meantime :)

Thanks again matey, we owe you one 🙏

Thanks :)
I'll create issue to emotion repo a bit later.
Always glad to help with some perf stuff 😃

@housseindjirdeh
Copy link
Collaborator

housseindjirdeh left a comment

SO SO good, can't thank you enough for this 🚀 🚀

return template(html, ids, css);
};

const renderingMethod = typeof window === 'undefined' ? renderOnServer : renderOnClient();

This comment has been minimized.

@housseindjirdeh
return o;
};

export const extractCritical = createExtractCritical(emotion);

This comment has been minimized.

@housseindjirdeh

housseindjirdeh Sep 22, 2018

Collaborator

Can't have been the easiest to dive in and find out exactly what the issue was. Appreciate it my friend 🙏 We can definitely remove this and use the upstream method once it's fixed within emotion, but this is perfect for now

width: 100%;
}
`;

This comment has been minimized.

@housseindjirdeh

@housseindjirdeh housseindjirdeh merged commit e958c65 into GoogleChromeLabs:master Sep 22, 2018

1 check passed

cla/google All necessary CLAs are signed
@denar90

This comment has been minimized.

Copy link
Contributor Author

denar90 commented Sep 22, 2018

🎉 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.