aphrodite
aphrodite copied to clipboard
Khan
Styles are injected after componentDidUpdate.
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: https://github.com/facebook/react/pull/6816
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?
(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 https://github.com/Khan/aphrodite/issues/76#issuecomment-220695582
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
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.
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).
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.
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.
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 https://github.com/Khan/aphrodite/issues/76#issuecomment-220857463
@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.
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.
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?
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.
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
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.
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?
@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
@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.
@xymostech Are you talking about flushToStyleTag vs setTimeout or flushToStyleTag vs insertRule?
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.
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
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?
@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?
@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',
},
});
@xymostech The code @butchler pasted in was originally introduced to avoid a flush of unstyled content (that only happened on Safari though).
@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().