Regression: GlamorousComponent being a class instead of stateless function causes issue with wrapped inputs

[danielberndt]

• glamorous version: 3.6.0

Check this simplified example of my code: https://codesandbox.io/s/L8OYqz1Wv

Since the result of glamorous(Comp) is now a class rather than a function, react now remounts the component on each rerender causing inputs to break. (Just start typing in the example. This used to work before introducing the theming support).

I am aware that the code-example can be rewritten quite easily to make it work, but the form framework I'm using encourages that pattern of the code example.

And finally: Thanks for this awesome library! This actually is the first time I'm feeling like I can stick with a styling solution for a long long time 😃

[kentcdodds]

Thank you for filing this bug! What do you think we need to do to fix it?

[danielberndt]

Hm... not too sure about how easy this is gonna be, but one approach would be to not return a new class, but something like this instead:

return function GlamorousComponent(props) {
  return (
    <GetTheme>{theme => doTheUsualStuff(props, theme)}</GetTheme>
  )
}

[danielberndt]

Or even

return function GlamorousComponent(props) {
  withTheme(({theme}) => doTheUsualStuff({theme, ...props}))
}

Which also would get rid of all the duplicated code for theme subscriptions!

[kkemple]

withTheme returns a react class as well, would that put us back in the same ⛵️ ?

[danielberndt]

oh, yes. You're absolutely right. So one work-around combining both approaches could be:

const GetTheme = ({children}) => withTheme(({theme}) => children(theme))

and then using it as described above:

return function GlamorousComponent(props) {
  return (
    <GetTheme>{theme => doTheUsualStuff(props, theme)}</GetTheme>
  )
}

or not even loading the theme from context if the props contain a theme already

return function GlamorousComponent(props) {
  return props.theme ? (
     doTheUsualStuff(props, props.theme)
  ) : (
    <GetTheme>{theme => doTheUsualStuff(props, theme)}</GetTheme>
  )
}

Note that the last example would lead to remounting the underlying component when props.theme changes its truthyness

[kokjinsam]

Would this change affect this.refs since stateless component doesn't support that?

[danielberndt]

yes, it would affect this.refs introduced since the theme support was introduced. (Wasn't possible before as well, I believe)
It's possible though to proxy refs like this: https://codesandbox.io/s/pQ529PA0X

[vesparny]

Wow, is there any technical difference between class and stateless function components?
I was not aware of that

[kentcdodds]

Ok, after looking at this more closely, I don't think that this is something I want to complect the codebase to support directly. However! There is a relatively simple workaround that you could use while still preserving the API you're looking for. For example:

import React from 'react';
import { render } from 'react-dom';
import glamorous from 'glamorous';
import memoize from 'fast-memoize';

const getComponent = memoize(Comp =>
  glamorous(Comp)({
    border: '1px blue solid',
  }),
);

const wrapInput = (Comp, props) =>
  React.createElement(getComponent(Comp), props);

class App extends React.Component {
  state = { value: '' };

  render() {
    return (
      <div>
        <h2>Form</h2>
        {wrapInput('input', {
          value: this.state.value,
          onChange: e => this.setState({ value: e.target.value }),
        })}
      </div>
    );
  }
}

render(<App />, document.body);

I hope that's sufficient. I consider this to be kinda edge-casey. Feel free to disagree with me and I'd be happy to review a PR that doesn't make things much more complex.

Good luck! And I'm glad you like glamorous :)

[danielberndt]

Okay, thank's for showing a possible workaround! :)

Some comments:

• One more thing that I've noticed caused by the change to class was that this no longer works:
  <div>{glamorous(Link)(style)({to: "/"})}</div>, instead it has to written as <div>{React.createElement(glamorous(Link)(style), {to: "/"})}</div> not a big loss, but having shortcuts can be nice of course :)
• I believe that some of the efforts of making glamorous smaller (Make glamorous smaller #42) could maybe lead to refactoring the index.js into smaller parts to allow for a more modular setup. This could e.g. allow the user to not include the theme related stuff, if themes are not being used. Such a refactoring could maybe allow to get back to a function-based (vs class-based) approach for the GlamorousComponent without sacrificing readability.
  I guess by now I've familiarised myself enough with the code base to try setting up a PR, but this of course will be something fairly invasive!

[kentcdodds]

this of course will be something fairly invasive!

Understood. I expect a lot of the work for #42 to be pretty invasive. If we can do it in several smaller PRs that would make reviewing faster and easier :) I'd love to see some refactor PRs that don't actually necessitate a new release, but make it so making new changes is easier 👍