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

react-fela: Initial render missing CSSRules because of `componentDidMount` #532

Closed
hkjorgensen opened this issue Mar 13, 2018 · 3 comments
Closed

Comments

@hkjorgensen
Copy link
Contributor

@hkjorgensen hkjorgensen commented Mar 13, 2018

Type: Bug / Feature / Infrastructure / Docs / Question

Packages Version
fela 6.1.4
react-fela 6.2.4

Description

react-fela offers a provider to wrap around a react render tree. The provider relies on the hook componentDidMount. Relying on the componentDidMount has some drawbacks. The initial rendered HTML has all the Fela generated class names - but the stylesheet is missing. I imagine this could lead weird rendering flashes for some people. I didn't notice this until debugging the following issue.

Imaging the following application tree:

const App = () => (
  <Provider>
    <MyComponent />
  </Provider>
)

MyComponent:

const Title = createComponent(() => ({ padding: '10px' }), h1)

class MyComponent extends Component {
  componentDidMount() {
     // perform operation that relies on measuring the rendered HTML.
  }
  render() {
     return <Title>Hello world</Title>
  }
}

There is a problem because of the order componentDidMount is called with React. Childrens componentDidMount is called before parents. This is the order:

  1. <MyComponent /> -> componentDidMount
  2. <Provider /> -> componentDidMount

At the time <MyComponent /> -> componentDidMount is called there are no Fela CSSRules available in the DOM and any measuring is likely to be invalid. In this case, <MyComponent /> should wait for the <Provider /> to invoke the componentDidMount that will do the initial render and add CSSRules to the DOM. That would be another Fela specific hook and not ideal.

React components depending on componentDidMount for measuring rendered HTML are common for animations and tools like react-textarea-autosize

Any rule changes after the first/initial render are o.k. because the <style /> has been inserted into the DOM and Fela renders happens immediately.

Proposal

A potential solution is moving the first render call to either the constructor or to the componentWillMount. According to the React Docs, componentWillMount might not be the right solution. I don't see any downside to do the initial render before mounting.

❤️ Thanks to @rofrischmann and everyone else who contributed - I love Fela and Fela's approach styling!

@robinweser

This comment has been minimized.

Copy link
Owner

@robinweser robinweser commented Mar 13, 2018

So you're not using SSR, am I right? As it would rehydrate the style.
componentWillMount is possible if you're only on the client, but using it on the server would throw as render(renderer) from fela-dom requires an actual DOM. We could add the same check to willMount as well, so if you're only rendering on the client, is injects the style on willMount, but if you're rendering on the server it won't throw.

Doing it in the constructor would have one downside. Every rule would be injected separately, while onDidMount (and willMount) we can inject all styles at once.

And of course, thanks for all the kind words! Are you using Fela for any cool project and want to add it to the list of users btw. :P?

@hkjorgensen

This comment has been minimized.

Copy link
Contributor Author

@hkjorgensen hkjorgensen commented Mar 13, 2018

@rofrischmann

So you're not using SSR, am I right? As it would rehydrate the style.

Correct, the current project is not using SSR.
Correct, if using SSR the issue mentioned above wouldn't happen :-)

My proposal would be to

ProviderFactory.js

- componentDidMount(): void {
+ componentWillMount(): void {
    if (this.props.renderToDOM && hasDOM(this.props.renderer)) {
      render(this.props.renderer)
    }
  }

The hasDOM() helper would guard it against trying to render server-sider. How does that sound @rofrischmann ?

I use it on all projects, most notably at Zendesk where we use it in the main product. I'll add it to the list!

@hkjorgensen hkjorgensen mentioned this issue Mar 14, 2018
7 of 7 tasks complete
@HugoGiraudel

This comment has been minimized.

Copy link
Contributor

@HugoGiraudel HugoGiraudel commented Apr 11, 2018

So the changes in 4c3358c are soon going to be a problem. As per 16.3, componentWillMount is entering deprecation phase in favor of componentDidMount.

Right now, it means it is not realistically possible to use React.StrictMode in a codebase using Fela because it produces a swarm of warnings.

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