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

Styles are injected after componentDidUpdate. #76

Open
pvolok opened this Issue May 20, 2016 · 38 comments

Comments

Projects
None yet
@pvolok
Copy link
Contributor

pvolok commented May 20, 2016

I'm trying to use aphrodite in my project. Everything works great so far except one scenario. I have a Tooltip component that measures children dimensions in componentDidMount and componentDidUpdate. But actual css gets injected after these functions were called. What's worse is that sometimes styles might be already injected, so dom element have unpredictable styling in above functions.

I created an issue on react to discuss how react might help with this problem: facebook/react#6816

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented May 20, 2016

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?

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented May 20, 2016

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

@matchu

This comment has been minimized.

Copy link

matchu commented May 20, 2016

(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 notifications@github.com
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)

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented May 21, 2016

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 notifications@github.com 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

@matchu

This comment has been minimized.

Copy link

matchu commented May 21, 2016

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.

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented May 21, 2016

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).

@matchu

This comment has been minimized.

Copy link

matchu commented May 21, 2016

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.

@jlfwong

This comment has been minimized.

Copy link
Collaborator

jlfwong commented May 22, 2016

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.

@matchu

This comment has been minimized.

Copy link

matchu commented May 22, 2016

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 notifications@github.com 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)

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented May 23, 2016

@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.

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented May 31, 2016

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.

@sophiebits

This comment has been minimized.

Copy link

sophiebits commented May 31, 2016

I commented on your React PR.

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented Jun 1, 2016

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?

@matchu

This comment has been minimized.

Copy link

matchu commented Jun 1, 2016

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.

@qimingweng

This comment has been minimized.

Copy link

qimingweng commented Jun 3, 2016

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

@pvolok pvolok referenced this issue Jun 6, 2016

Closed

Try insertRule #83

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented Jun 6, 2016

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.

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented Jun 6, 2016

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

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Jun 8, 2016

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?

@qimingweng

This comment has been minimized.

Copy link

qimingweng commented Jun 8, 2016

@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

@jlfwong

This comment has been minimized.

Copy link
Collaborator

jlfwong commented Jun 8, 2016

@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.

@pvolok

This comment has been minimized.

Copy link
Contributor

pvolok commented Jun 8, 2016

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

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Jun 9, 2016

@pvolok I wasn't talking about insertRule.

@azizhk

This comment has been minimized.

Copy link

azizhk commented Jun 19, 2016

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.

@matchu

This comment has been minimized.

Copy link

matchu commented Jun 19, 2016

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 notifications@github.com
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 .
@butchler

This comment has been minimized.

Copy link

butchler commented Sep 29, 2016

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?

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Sep 29, 2016

@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?

@butchler

This comment has been minimized.

Copy link

butchler commented Sep 29, 2016

@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',
  },
});
@brainshave

This comment has been minimized.

Copy link

brainshave commented Sep 29, 2016

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

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Sep 29, 2016

I would just use inline styles for that specific property

@butchler

This comment has been minimized.

Copy link

butchler commented Sep 29, 2016

@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().

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Sep 29, 2016

@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.

@Munksey

This comment has been minimized.

Copy link

Munksey commented Oct 2, 2016

@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.

@xymostech

This comment has been minimized.

Copy link
Contributor

xymostech commented Oct 3, 2016

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?

@Munksey

This comment has been minimized.

Copy link

Munksey commented Oct 4, 2016

@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?

@jlfwong

This comment has been minimized.

Copy link
Collaborator

jlfwong commented Oct 4, 2016

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).

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Oct 4, 2016

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.

@jlfwong

This comment has been minimized.

Copy link
Collaborator

jlfwong commented Oct 4, 2016

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.

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Oct 4, 2016

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 :)

This was referenced Oct 9, 2016

lencioni added a commit that referenced this issue Mar 15, 2018

Expose flushToStyleTag
v2 switched from webpack to rollup for its build. As part of this, the
individual files that were previously importable by intrepid spirits
gave access to some functions that were not explicitly exposed as part
of Aphrodite's public API.

At least one of these is cited in some issues as a workaround for some
edge cases like #76, and is used by projects like
react-with-styles-interface-aphrodite to allow folks to work around some
of these same types of issues.

To allow folks relying on this function to update to v2, I am adding
this as an export.

lencioni added a commit that referenced this issue Mar 15, 2018

Expose flushToStyleTag
v2 switched from webpack to rollup for its build. As part of this, the
individual files that were previously importable by intrepid spirits
gave access to some functions that were not explicitly exposed as part
of Aphrodite's public API.

At least one of these is cited in some issues as a workaround for some
edge cases like #76, and is used by projects like
react-with-styles-interface-aphrodite to allow folks to work around some
of these same types of issues.

To allow folks relying on this function to update to v2, I am adding
this as an export.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment