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

Idea: Debounce adding of styles for better performance #5

Closed
jhnns opened this issue Jan 27, 2014 · 15 comments
Closed

Idea: Debounce adding of styles for better performance #5

jhnns opened this issue Jan 27, 2014 · 15 comments

Comments

@jhnns
Copy link
Member

jhnns commented Jan 27, 2014

I'm thinking about this issue quite a while but haven't found the time to write it down.

My co-worker was working on an app (without webpack) where he wanted to define style rules per component. These styles should only be applied if the component is currently in use, so he added and removed the style nodes accordingly.

While the theory is very nice, it has a serious performance impact. Because when a style node is added or removed the browser needs to recalculate all styles and to reflow the whole document. So when a page changed, the browser needed to recalculate and to reflow several times.

I was wondering if this problem could be solved by the style-loader. I was thinking about debouncing the call where the <style> node is actually added to the DOM.

This feature needs to be tested carefully to avoid FOUTs (Flash of Unstyled Text) or other ugly problems when unstyled content is added to the DOM.

@dashed
Copy link

dashed commented Jan 27, 2014

What about this? https://github.com/wilsonpage/fastdom

@jhnns
Copy link
Member Author

jhnns commented Jan 28, 2014

Interesting module. How could the fastdom instance be shared between the loader and the app?

@sokra
Copy link
Member

sokra commented Jan 28, 2014

With fastdom:

fastdom.write(function() {
  require("./style.css");
});

Style added and removed only when component is used:

var style = require("./style.usable.css"); // and bind "style/usable!css" to ".usable.css"
constructor {
  style.use();
}
destructor {
  process.nextTick(function() {
    style.unuse();
  });
}

Nothing to do in this loader.

@jhnns
Copy link
Member Author

jhnns commented Jan 29, 2014

Yep, you're right. This should be done on application level.

But this is a nice pattern. Imho it should be documented in the readme.

@petehunt
Copy link

petehunt commented Mar 1, 2014

I'm not sure if webpack should solve this either, but how about the following situations?

  1. A stylesheet is repeatedly use()d and unuse()d, causing the stylesheet to be added and removed a lot. I believe this could be a common case if you're swapping out a top-level component for another one: all the common stylesheets will be unloaded and reloaded unnecessarily.
  2. I'm very careful to call use() and unuse() at the right time, but some dependency that uses webpack that I don't control doesn't and thrashes my layout.

It could be cool if use() and unuse() were async went into a queue with a pluggable batching/flushing strategy, so I could plug it into React's render loop or fastdom or something for free.

These may be the responsibility of the framework; not sure. But is the first problem even solvable with the current API?

@sokra
Copy link
Member

sokra commented Mar 1, 2014

Assuming a module that uses the reference-counted API like this:

var style = require("./style.useable.css");
style.use();
style.unuse();

And the application uses the recommended configuration:

{
  module: {
    loaders: [
      { test: /\.css$/, exclude: /\.useable\.css$/, loader: "style!css" },
      { test: /\.useable\.css$/, loader: "style/useable!css" }
    ]
  }
}

It's possible for the application to apply any additional strategy that does batching (etc.). Just prepend a loader that decorates the public API of the generated module:

{
  module: {
    loaders: [
      { test: /\.css$/, exclude: /\.useable\.css$/, loader: "style!css" },
      { test: /\.useable\.css$/, loader: "batching-style-loader!style/useable!css" }
    ]
  }
}
// batching-style-loader.js
module.exports = function() {};
module.exports.pitch = function(remainingRequest) {
  return "module.exports = require(" +
    JSON.stringify(path.join(__dirname, "batching-style-decorator.js")) +
    ")(require(" + JSON.stringify(remainingRequest) + "));";
};
// batching-style-decorator.js
var queue = [];
module.exports = function decorate(style) {
  function ref() {
    // Apply now
    style.ref();
    queue.forEach(function(style) {
      style.unref();
    });
  }
  function unref() {
    process.nextTick(function() {
      // Unapply on the next ref... or whatever
      queue.push(style);
    });
  }
  return {
    ref: ref,
    use: ref,
    unref: unref,
    unuse: unref
  };
};

@dashed
Copy link

dashed commented Mar 2, 2014

Use fastdom over process.nextTick to eliminate layout thrashing :)

@necolas
Copy link

necolas commented Aug 31, 2014

Have you looked into using the disabled attribute for the module's associated style tag, instead of removing the element? Perhaps that can avoid the issue of "all the common stylesheets will be unloaded and reloaded unnecessarily"

@jhnns
Copy link
Member Author

jhnns commented Sep 1, 2014

I don't know such a disabled attribute

@necolas
Copy link

necolas commented Sep 1, 2014

Link: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style#attr-disabled

(Toggling the disabled attribute on link elements is how we did theme-switching in TweetDeck.)

@dashed
Copy link

dashed commented Sep 1, 2014

I like that suggestion. Seems the disabled attribute is only accessible in js via HTMLStyleElement.

Example: http://jsbin.com/reneca/2/edit

Explicitly adding it as an attribute in the tag won't work; I think is non-standard:

    <style type="text/css" disabled>
      body {
        color:red;
      }
    </style>

@jhnns
Copy link
Member Author

jhnns commented Sep 1, 2014

Interesting... according to this answer <style>-tags don't have a disabled attribute, but the corresponding HTMLStyleElement has a disabled-property.

@necolas
Copy link

necolas commented Sep 1, 2014

yeah, we'd only need to use the DOM API

@michael-ciniawsky
Copy link
Member

@jhnns @necolas @dashed Can this one be closed? Is disable still a considerable feature?

@jhnns
Copy link
Member Author

jhnns commented Mar 10, 2017

It can be closed, but a note in the README would be good that the styles should be applied in batches when used in production (for instance via fastdom).

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

No branches or pull requests

6 participants