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

Allow registerStyle after render... #13

Closed
tracker1 opened this issue Jan 11, 2016 · 19 comments
Closed

Allow registerStyle after render... #13

tracker1 opened this issue Jan 11, 2016 · 19 comments

Comments

@tracker1
Copy link

This may sound weird, but I'd like to be able to support adding styles after the style.element is rendered. I currently get the following error.

Inline styles must be registered before render

However, it looks like StyleElement listens for event changes to force a rerender... so I should be able to register styles dynamically after the initial render. I understand this may be problematic for a server-side render, but I'd like to be able to change my themes dynamically after the application is loaded, and register themes as such.

Not to mention, with HMR, I'd like to be able to edit styles on my component(s), this error forces me to refresh the entire application/page in order to see style changes.

@blakeembrey
Copy link
Owner

This should already be working with HMR, it used to work perfectly (haven't been using with React 0.14 yet). The feature you describe used to actually be well-documented and is still supported, but you can not update styles within a render loop (the React update model blocks this). If you want to do this, try using componentWillMount and componentWillUnmount.

Edit: Let me know if this is the issue, of course. If it's broken, this is a bug.

Edit 2: A better phrase would be "Inline styles can not be registered during render" and I will make that change.

@blakeembrey
Copy link
Owner

Released a new minor version with the log still in place, but moved and no longer restricting a render (just note, it will crash React, but now it's transparent why).

@hoppula
Copy link

hoppula commented Jan 18, 2016

I'm also having trouble hot reloading styles using react-transform-hmr.

I have a root component that renders the <Style.Element /> and when I'm changing styles in children components, the style class hashes will get updated but the root component's style element does not pick up the changes.

@hoppula
Copy link

hoppula commented Jan 18, 2016

After a bit of debugging it seems that with react-transform-hmr FreeStyleComponent's componentWillUpdate will not get called, but it does for wrapped Component.

@tracker1
Copy link
Author

@blakeembrey Thanks for making that change... wish it were possible, since I'd like to be able to use it for static methods... that said, if the message indicated one should use componentWillMount and componentWillUnmount, that would alleviate this issue entirely.

@blakeembrey
Copy link
Owner

@hoppula @tracker1 If you guys have a project set up you can share, I would love to patch the issue.

@tracker1 That's a great suggestion, thanks!

@hoppula
Copy link

hoppula commented Jan 18, 2016

@blakeembrey Great! I probably should've opened another issue for this as it's not directly related to original topic, just saw HMR being mentioned :) Anyways, I created a sample case from react-transform-boilerplate: https://github.com/hoppula/react-free-style-hmr

Editing textStyle in src/App.js will change the class hash for h1 element but doesn't update styles in StyleRoot component.

I just tested the exact same setup with react-hot-loader and it works, so maybe I'll go with react-hot-loader for now, there's just that deprecation notice in react-hot-loader repo that makes me a bit worried about the future.

@blakeembrey
Copy link
Owner

@hoppula Thanks so much for this! Give me a bit of time to check it out, but that's good to see. You might be right about HMR, I believe the last time I was doing this was with react-hot-loader, but it should be easy enough to figure out why things aren't working with HMR.

@tracker1
Copy link
Author

@blakeembrey my thought was something like...

export default function renderMyComponent(props) {
  var btnClass = props.freestyle.registerStyle(props.theme.button.primary);

  return <div>
    ...
    <button className={btnClass} onClick={props.onClick}>Do Something</button>
  </div>;
}

Essentially a static class that used/generated themes using freestyle against a themes property that is passed in... Using redux, themes can be changed/loaded/reloaded, so may change after the first render (which works) on a re-render, or in HMR. When the className is the same (from cache) it works... but if I update a component and HMR fires, or if my theme changes, it blows up with the parent error...

If I can find time, I'll try and work up a simple demonstration, something similar to @hoppula above. I worked around the issue, for now by using inline styles where they are changing... I'm still pretty early on with a proof of concept at work, so didn't really dig into this.

@blakeembrey
Copy link
Owner

@tracker1 Sorry for the huge delay, a lot come up recently. Technically this currently is supported, but it's part of the context and requires you to register and remember to unregister. I'm going to figure out if I can make a helper method that hooks into the current elements render lifecycle (perhaps you know?). Right now I'm going to figure out why HMR isn't working with the shared demo - thank you for it!

@blakeembrey
Copy link
Owner

Is it possible that you can generate the theme hashes up front? The problem is you can't update an unrelated element within the same update loop. One workaround would be to ditch having React manage it and instead use the DOM node directly for updates.

@blakeembrey
Copy link
Owner

@blakeembrey
Copy link
Owner

As an aside, it looks like neither works with the new stateless functional components (correct me if I'm wrong)?

@tracker1
Copy link
Author

tracker1 commented Feb 9, 2016

@blakeembrey I believe you are correct, in that neither works well for stateless/static component methods... My desire was to be able to work with edit/change and allow user selection of themes without having to reload the page... since the theme would be loaded via redux higher up, and passed in via props, it made sense to me... I plan to use mostly static (stateless) components.

@tracker1
Copy link
Author

tracker1 commented Feb 9, 2016

One option, is you could do something similar to what I am for managing my themes, in order to create a context for your <Style.Element /> ... As opposed to a style element component, you create a Style.Container ...

<Style.Container>
   <Child />
<Style.Container>

Style.Container could deliver a context.style or context.freestyle Then self-render the style element after children.

Any Child component only has to register that it needs freestyle and can use context.freestyle.register...

It would invert the scope, but allow for safe changes at render...

// Child.jsx - static component
export default function child(props, context) {
  var class = context.freestyle.registerStyle(...); //some computed style based on other props.
  return <div>...</div>;
}

//register for the context type
childRender.contextTypes = {
    freestyle: React.PropTypes.object.isRequired
};

I am actually doing something similar to pass down my theme... Would be nice, if you could have a "theme" property on the container component, that was also passed via context... as a convenience method.

@blakeembrey
Copy link
Owner

Thanks for the update. There's a few implications of doing this, but I'll keep it in mind. Here's my concerns though:

  • Currently the context contributes to a global instance that (preferably) maintains the style element. Won't this cause multiple style elements to be changed as you nest deeply?
  • Using registry style promotes never detaching, which is ok here but not when a style is propagating upward (unless you don't care about dead style removal)
  • And finally, doesn't this result in a lot of style element duplication?

Of course, I could be reading the proposal wrong. I do think supporting this is a priority, would love a clean API. We can think about how to support this together. Here's an initial thought:

  • Every component creates its own Free Style instance and merges with the parent context on mount and detaches/destroy on unmount (makes sure dead styles aren't accidentally reused)
  • Change style element render to support next tick operations to avoid React render issue (maybe there's a better solution here?)
  • Finally, support Style.Container usage as an alternative to Style.component()

Does that sound correct?

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 13, 2016

I'm just re-reading this, and it seems everything in #13 (comment) is actually how it worked in 2.0 but it wouldn't have been acceptable for the edge-cases. I'm looking at releasing a 3.0 that uses free-style@2.0.0 - what changes do I need to make to make this work for everyone? I have refactored it so that changes registered on the component through the context attribute will automatically be detached when the component unmounts (no more manual style detaching management - should work better for stateless components).

However, I believe the restriction for needing to render styles outside of the render loop still exists because React won't let me re-render the style element itself while there's other updates still occurring. With that said, what's a reasonable workaround for React?

@blakeembrey
Copy link
Owner

I lied above. Turns out stateless components didn't run into that issue. I've updated the docs and added some tests. See 97d56f7.

@blakeembrey
Copy link
Owner

Actually, does anyone have experience testing re-renders? I just realised the reason I didn't hit the issue locally is probably because it's only doing the one render loop and the StyleElement always attaches last anyway.

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

3 participants