Styles are injected after componentDidUpdate. about aphrodite HOT 38 OPEN

Until that can be addressed, should this be added to the Caveats section of the README?

from aphrodite.

@jlfwong Yeah, I suck at explaining what I mean :)

I just ran this code:

componentDidMount() {
  const dom = ReactDOM.findDOMNode(this);
  console.log(dom.offsetHeight); // 10
  require('aphrodite/lib/inject').flushToStyleTag();
  console.log(dom.offsetHeight); // 10

  setTimeout(() => {
    console.log(dom.offsetHeight); // 14
  }, 0);
}

What I'm saying is I don't know (yet!) how we can get new styles applied in the current tick. I haven't investigate any further than this snippet. Maybe we can somehow force browser to parse and apply new css synchronously. I'll try to play with it later this week.

from aphrodite.

I'm now becoming more in favor of exposing flushToStyleTag(). People at KA have been running into problems with styles in mobile safari, where the styles aren't getting flushed in time which causes elements to reflow badly, or causes animations/transitions to not trigger. I think that adding a call to flushToStyleTag() in componentDidMount/componentDidUpdate will fix these problems.

Although, maybe the fact that the styles aren't being injected quickly enough in mobile safari (I think it's in a webview in an app which might make a difference) is a larger problem. Maybe we shouldn't be buffering styles in those cases, because we'll almost certainly get a FOUC?

from aphrodite.

To get around this problem, we are currently using a wrapper around Aphrodite that eagerly injects the styles as soon as they are defined, instead of waiting until they are used in a render function:

import * as Aphrodite from 'aphrodite/no-important';
import * as Logger from 'app/services/logger';

export function createStyleSheet(defs) {
  const styles = Aphrodite.StyleSheet.create(defs);

  Object.keys(defs).forEach(key => Aphrodite.css(styles[key]));

  return styles;
}

export function css(...args) {
  const classes = args.map(style => style && Aphrodite.css(style)).join(' ');

  if (process.env.NODE_ENV !== 'production' && args.length > 1) {
    Logger.warn(
      `\`css\` called with multiple arguments: ${classes}
This behaviour is sentitive to order in which classes are defined. Use with care.`
    );
  }

  return classes;
}

However, injecting the styles when they are defined means that we cannot use the multiple-argument form of Aphrodite's css() function, because it creates a new style and injects it on the fly. Instead, we discourage passing multiple argument to css() and manually merge styles when they are defined using Object.assign or object-rest-spread.

Personally, I would like to have the option to use a synchronous API for Aphrodite, where Aphrodite doesn't inject any styles until you call some kind of global injectAllStyles() function, after which you could render your app. This would be efficient because all styles would get inserted at once, and it would remove the problems with styles not being applied. However, such an API would require removing the ability to pass multiple arguments to css() and have it automatically merge the styles for you.

Would the Aphrodite team be willing to consider exposing an option to use an eager-loading or synchronous version of the API that disables the ability to pass multiple arguments to css(), for developers who are willing to merge the styles manually in order to avoid the problems sometimes caused by injecting styles asynchronously?

from aphrodite.

@butchler I would be personally opposed to such an API change. Not being able to pass multiple styles to css() gets rid of most of the things I like about Aphrodite!

Is there a reason why the setTimeout() hack doesn't solve your problems? I guess I'm mostly curious what problems you're seeing that is causing you to push against the Aphrodite API so much! Would something like the flushToStyleTag() API we talked about earlier that immediately flushes the styles solve your problems?

from aphrodite.

@butchler Would something like the flushToStyleTag() API we talked about earlier that immediately flushes the styles solve your problems? You would then call this right before you do the measurements.

from aphrodite.

Thanks for reporting this! At KA, we do a setTimeout(function() { /* measuring in here */ }, 0); in a couple of places to solve this exact problem, but you're right that it isn't super obvious that this is needed, and is confusing at best. @matchu is the one who ran into this, so probably has some valuable feedback.

We were talking about maybe adding a function like waitForStyles or something that you could pass a callback, and it would run the callback once all the styles had been injected (or immediately if there were no styles queued to be injected). Does that sort of interface sound reasonable to you?

from aphrodite.

(My feedback is that, yes, we should do this! :D Even if our first draft is
super naive, it'll help clarify that this is an intended part of the API.)

On Fri, May 20, 2016, 12:20 PM Kent C. Dodds [email protected]
wrote:

Until that can be addressed, should this be added to the Caveats section
https://github.com/Khan/aphrodite#caveats of the README?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#76 (comment)

from aphrodite.

In my opinion waitForStyles should be synchronous (will styles be applied synchronously after style tag injection?). It will make things simpler. What do you think?

I'd be happy to send a PR later today.

On May 20, 2016, at 11:53 PM, Emily Eisenberg [email protected] wrote:

Thanks for reporting this! At KA, we do a setTimeout(function() { /* measuring in here */ }, 0); in a couple of places to solve this exact problem, but you're right that it isn't super obvious that this is needed, and is confusing at best. @matchu is the one who ran into this, so probably has some valuable feedback.

We were talking about maybe adding a function like waitForStyles or something that you could pass a callback, and it would run the callback once all the styles had been injected (or immediately if there were no styles queued to be injected). Does that sort of interface sound reasonable to you?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

from aphrodite.

The only way for it to be synchronous is for the waitForStyles call to immediately flush the buffer, right? That seems bad for performance—it's the same reason that we even have a buffer and don't just flush on every single css call.

Even if the performance difference turned out to be small, I'd prefer the async API. The complexity difference seems almost nonexistent to me, and async gives us more options if we decide to change the implementation in the future.

from aphrodite.

I checked if injected styles are applied immediately. And the answer is no, unfortunately. So probably we can't even implement synchronous waitForStyle with current approach.

My idea of flushing styles is to follow react's process: call all render functions, then flush to the dom, and call all didComponentUpdates. If we know we need styles in didComponentUpdate, we can flush all css calls that are already batched. So aphrodite will flush as many times as react flushes (or less).

from aphrodite.

Hmm! It'd be nice to keep Aphrodite decoupled from React, so, even if we hooked into React, we should probably also offer a waitForStyles-like mechanism for non-React users, too.

from aphrodite.

I think the synchronous version should be fine, as long as you're calling it in componentDidMount or componentDidUpdate, and not calling it in render().

All the buffered CSS calls should've come from render(), so by the time you reach componentDidMount or componentDidUpdate for any of the components, the buffered CSS should be fully buffered for all components, so forcing a flush in componentDidMount won't break a large buffer of CSS into smaller buffers, it'll just flush earlier.

I checked if injected styles are applied immediately. And the answer is no, unfortunately. So probably we can't even implement synchronous waitForStyle with current approach.

@pvolok Can you expand upon this? I'm not sure what you mean here and what is making you suspect that the synchronous approach isn't workable.

from aphrodite.

Mm, true! I don't have an especially strong opinion in this case, then :)

On Sun, May 22, 2016 at 2:27 PM Jamie Wong [email protected] wrote:

I think the synchronous version should be fine, as long as you're
calling it in componentDidMount or componentDidUpdate, and not calling it
in render().

All the buffered CSS calls should've come from render(), so by the time
you reach componentDidMount or componentDidUpdate for any of the
components, the buffered CSS should be fully buffered for all
components, so forcing a flush in componentDidMount won't break a large
buffer of CSS into smaller buffers, it'll just flush earlier.

I checked if injected styles are applied immediately. And the answer is
no, unfortunately. So probably we can't even implement synchronous
waitForStyle with current approach.

@pvolok https://github.com/pvolok Can you expand upon this? I'm not
sure what you mean here and what is making you suspect that the synchronous
approach isn't workable.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#76 (comment)

from aphrodite.

I tried again the code from previous comment. And now new style is actually applied after flushToStyleTag(). Probably I did something wrong before. So just exposing flushToStyleTag would be enough. But still 3rd-party components won't call it before measuring. Maybe we should wait until someone from react core team comments on facebook/react#6816.

Also I found out that style.appendChild(cssText) scales not so good. Whereas style.sheet.insertRule() scales linearly.

from aphrodite.

I commented on your React PR.

from aphrodite.

Thanks!

Well, since react won't support it I don't think there is a way to flush the buffer by the time the first componentDidUpdate is called. I PRed a note about it (#79).

Should this issue be closed?

from aphrodite.

If we wanted tighter React integration, we could create a higher-order component or a mixin or some such, perhaps? I'm not sure how the React APIs work, so I don't actually know how feasible that is, but it seems like it oughta be straightforward?…

…that said, I'm not sure the overhead of declaring "this is an Aphrodite-aware component" is any lower than the overhead of just using Aphrodite's flushing API directly :/ depends what API we come up with.

from aphrodite.

What are your thoughts on exposing the option to synchronously inject styles on every css call. I realize this is bad for performance, but when working with other libraries that expect synchronicity in measuring components, this can be difficult. Take https://github.com/qimingweng/react-center-component, for example.

Another thought I have is this, and it's pretty early so I'm not sure if it's possible, but perhaps, for the react renderer, we can create a syntax like

render = () => {
  return aphroditeFlush(() => {
    return <div className={css(styles.style)}/>;
  });
}

This way, we can define the barriers of a flush cycle manually?

cc @moberemk @ianvdhSBM

from aphrodite.

I tried to use insertRule in each css call. If we use this approach, then styles will always be applied by the time componentDidMount and componentDidUpdate are called.

In chrome, if we initially render 100 classes, the performance is the same as with styleTag.appendChild. But there is one issue: if the browser can't parse rule (like ::-moz-focus-inner in chrome) it throws, so we have to wrap insertRule with try-catch, which adds about 25% performance penalty.

As our application continues to work, insertRule scales very well as the browser doesn't have to reparse css that was inserted before.

Would be cool if someone from the Khan Academy checked how it performs in real world application. I've added a PR with insertRule version: #83.

from aphrodite.

Oh. Nevermind. Tests are broken. I'll fix it tomorrow.

from aphrodite.

@xymostech Perhaps even include a react HOC to make this simpler, something like

import React from 'react';
import { flushToStyleTag } from 'aphrodite';

const flushAfterMount = (Component) => {
  return class Decorated extends React.Component {
    componentDidMount() {
      flushToStyleTag();
    }
    componentDidUpdate() {
      flushToStyleTag();
    }
    render() {
      return <Component {...this.props}/>;
    }
  }
}

Updated to include componentDidUpdate

from aphrodite.

@xymostech Any chance you have the benchmarks that led us to go down the buffering path in the first place still lying around? Now that a larger part of khanacademy.org is using Aphrodite, we might be able to benchmark something more realistic and see how bad not buffering at all really is.

from aphrodite.

@xymostech Are you talking about flushToStyleTag vs setTimeout or flushToStyleTag vs insertRule?

from aphrodite.

@pvolok I wasn't talking about insertRule.

from aphrodite.

Just a random idea: How about something like:

class App extends Component {
  render() {
    return <div {...instantStyles(styles.red)}>
      This is red.
    </div>;
  }
}
const styles = StyleSheet.create({
  red: {
    backgroundColor: 'red',
    width: '200px'
  }
}

instantStyles returns an object with {className:'red-abc'}
Now I think aphrodite knows that what styles are already there in the <style> tag and what are not, so it sees that this is a new style and the developer has asked for instantStyles so return a object with style as well.

{
  className: 'red-abc'
  style: {
    backgroundColor: 'red',
    width: '200px'
  }
}

In next render this gets removed. And in the case of SSR only className key should be returned in the object. One down side is that pseudo styles ::before and ::after would get neglected.
Shoot this down if pointless.

from aphrodite.

Oh! This is interesting!

For a while I've been vaguely interested in a higher-level API: declare
"appearances" in terms of CSS classes (managed by Aphrodite) and style
attributes, and offer the ability to merge them (like Aphrodite's css
function). I like it for the consistent interface it provides—I don't have
to care whether an appearance involves CSS classes or standalone styles in
order to apply it to an element—but it's interesting that it would also
offer this additional optimization opportunity.

I think this makes more sense as a layer on top of Aphrodite's current
functionality, but I'm pretty interested in that additional layer…

On Sun, Jun 19, 2016 at 9:36 AM Aziz Khambati [email protected]
wrote:

Just a random idea: How about something like:

class App extends Component {
render() {
return <div {...instantStyles(styles.red)}>
This is red.
;
}
}const styles = StyleSheet.create({
red: {
backgroundColor: 'red',
width: '200px'
}
}

instantStyles returns an object with {className:'red-abc'}
Now I think aphrodite knows that what styles are already there in the

<style> tag and what are not, so it sees that this is a new style and the developer has asked for instantStyles so return a object with style as well. { className: 'red-abc' style: { backgroundColor: 'red', width: '200px' } } In next render this gets removed. And in the case of SSR only className key should be returned in the object. Shoot this down if pointless. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com//issues/76#issuecomment-227006852, or mute the thread https://github.com/notifications/unsubscribe/AADnUl7ai9HkiM0kWxHN3LaM9mDxXBJ9ks5qNXAcgaJpZM4IjATS .

from aphrodite.

@xymostech We have a component that automatically resizes text returned by our API so that it fits within a certain maximum height. It needs to measure the height of the container and modify the font sizes, so if we used setTimeout() to make it asynchronous, it would show the text at its original size for one frame and then resize it the next frame, which would be really jarring for the user.

Maybe there's a better way of solving our particular problem that doesn't require dynamically resizing the text based on the container height, but more generally the asynchronous API will not work if you want to both measure the styles of a DOM component and modify some styles based on those measurements before the initial render. (I know that's a really specific situation, but it happened to us =P)

Regarding not being able to pass multiple arguments to css(), I thought it would be really annoying at first, but as long as you use object-rest-spread it's actually not that bad to just manually merge the style definition objects yourself when you create the stylesheet. For example, you could do something like:

const BASE_STYLES = { color: 'black', backgroundColor: 'white' };

const styles = StyleSheet.create({
  changeColor: {
    ...BASE_STYLES,
    color: '#333',
  },
  changeBackground: {
    ...BASE_STYLES,
    backgroundColor: '#eee',
  },
});

from aphrodite.

@xymostech The code @butchler pasted in was originally introduced to avoid a flush of unstyled content (that only happened on Safari though).

from aphrodite.

I would just use inline styles for that specific property

from aphrodite.

@kentcdodds That wouldn't work for the text-resizing component I mentioned earlier, because it is intended to be a generic component that you can use in many places, so you'd have to explicitly define the fontSize with an inline style for ever component inside of every instance of the text-resizing component. This is further complicated, because a lot of the font sizes might be inherited, but you'd still have to explicitly define it on each element with an inline style, making it much harder to change the inherited font size later.

This is a very specific case and a very hacky component, though, so I don't think the normal Aphrodite API should be changed just for this. However, it would be nice to have the option to use a synchronous API if you want to avoid all of these kinds of issues at the expense of losing the multi-argument version of css().

from aphrodite.

@xymostech, I work with @butchler and I really like the solution we have. All of our merging happens within each StyleSheet.create call, so we don't have to merge using the 'css' method. Also, since we don't use higher order styles, all calls to StyleSheet.create happen after our bundle file is loaded.

I do think adding the flushToStyleTag would give people more control and it would be something that we would use on top of our current solution to add the styles before we render anything. It would make the process more explicit.

However, I think it would be nice if StyleSheet.create had an options field that could be used to preload certain styles. Then, perhaps there could be another option to flush on call? Let me propose a couple of possibilities here:

// Flushes to style sheet after being called.
// Eager loads red style, green style and merged red, green style.
StyleSheet.create({
  red: { color: 'red' }),
  green: { color: 'green' }),
}, {
  eagerLoad: ['red', 'green', ['red', 'green']],
  flushToStyleTag: true,
});

// Doesn't flush to style on call.
// Eager loads red and green.
StyleSheet.create({
  red: { color: 'red' }),
  green: { color: 'green' }),
}, {
  eagerLoad: true,
});

I personally don't like having parameters take different types (ex: eagerLoad being true and an array of keys), but I am just trying to spark some ideas for making StyleSheet.create a little more convenient.

from aphrodite.

I'm pretty cautious of adding options to StyleSheet.create, I feel like it opens a can of worms that we don't want to deal with. So far we've managed to explicitly avoid that. Your example could be accomplished with flushToStyleTag() by doing something like

const styles = StyleSheet.create({
  red: { color: 'red' }),
  green: { color: 'green' }),
});

css(styles.red);
css(styles.green);

flushToStyleTag();

Is that too ugly?

from aphrodite.

@xymostech I completely understand what you mean. Especially since what we want can be accomplished without the options field.

I don't think what you wrote is ugly. The code you put is pretty much what our wrapper code does, minus the flushToStyleTag call. My concern was mostly about explicitly supporting what I think is a useful pattern.

I think for performance, we would probably call flushToStyleTag once (after all modules have loaded or before we run any tests in our test runner), to maximize performance. Although, I'd be interested in seeing what kind of performance hit happens if we were to wrap css and call flushToStyleTag every time.

Since it seems like there could potentially be multiple patterns for implementing eager loading, perhaps it is best just to provide the flushToStyleTag and leave that implementation detail up to everyone. Perhaps if there is more feedback in the future and everyone seems to follow a common pattern, there can be more helper methods added later?

from aphrodite.

I'm for the flushToStyleTag approach, though we need to be careful about the implications of that on server-side rendering (particularly, flushToStyleTag should be a no-op when run server-side).

from aphrodite.

I think I'd prefer it if flushToStyleTag threw an (informative) error on the server. Fail fast. I'd prefer to not allow people to think they're doing one thing when it actually results in a no-op.

from aphrodite.

Hmm, I'm unsure.

On the one hand, I can't think of any use cases where flushToStyleTag should be used inside a render() call. On the other hand, nothing would break if you did. I'm having trouble thinking of code that you would write and expect to work server-side and client-side that would work if flushToStyleTag did a real flush and fail if it was a no-op.

The only use cases where it seems to me to matter if styles have been flushed or not is doing DOM measurements, and you can't do those server-side anyway.

from aphrodite.

I just don't like the idea of having my code say: doThisThing() and have it actually do anything but doThisThing(). But maybe I'm not thinking of this correctly.

Disclaimer, I'll probably not use this API (certainly not on the server), so my opinion is pretty much irrelevant. Feel free to ignore me :)

from aphrodite.