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

Theming via context in a stateless component throws error. #153

Closed
DavidBachmann opened this issue Mar 24, 2017 · 5 comments
Closed

Theming via context in a stateless component throws error. #153

DavidBachmann opened this issue Mar 24, 2017 · 5 comments

Comments

@DavidBachmann
Copy link

Theming via context in a stateless component throws the following error:

Expected context to not come from the closest scope.

Here's an example of what I'm trying to do:

const Button = ({children}, context) =>
  <button className="TestButton">
    {children}

    <style jsx>{`
      .TestButton {
        color: ${context.theme.color};
      }
    `}</style>

  </button>

Button.contextTypes = {theme: React.PropTypes.object}
throw er; // Unhandled 'error' event
^
SyntaxError: /dev/test/contexttest/Button.jsx: Expected `context` to not come from the closest scope.
Styled JSX encourages the use of constants instead of `props` or dynamic values which are better set via inline styles or `className` toggling.
See https://github.com/zeit/styled-jsx#dynamic-styles
 3 |     <style jsx>{`
 4 |       .TestButton {
> 5 |         color: ${context.theme.color}
   |                  ^
 6 |       }
 7 |     `}</style>
 8 |   </button>;
   at File.buildCodeFrameError (/dev/test/contexttest-next/node_modules/babel-core/lib/transformation/file/index.js:427:15)

Turning the component into a class works, but I would really appreciate having my stateless components theme-able.

Is this a bug, a missing feature, or am I missing something?

Thanks

@DavidBachmann DavidBachmann changed the title Theming via context in a stateless component throws error: Expected context to not come from the closest scope. Theming via context in a stateless component throws error. Mar 24, 2017
@giuseppeg
Copy link
Collaborator

@DavidBachmann we don't allow dynamic styles on purpose see this and this. We are planning on adding support for dynamic styles though so stay tuned! :)

P.S. the fact that it works with classes is a bug, thanks! I need to fix it :)

@eirikurn
Copy link

eirikurn commented Mar 24, 2017

I understand the reason for this check. However, for widget libraries (think Material UI), it makes sense to get theme variables from the context instead of global variables. Consider a <ThemeProvider> where the user can override parts of the theme. Even though a variable has a local scope, it can be enforced to be constant, just like outer scope variables can be dynamic.

Would you consider a PR for an optional babel flag that silences this error? For projects/libraries that "know better". Something like:

{
  "plugins": [
    ["styled-jsx/babel", {
      "scopeCheck": false,
    }]
  ]
}

Or maybe there could be another attribute/value like #81 that disables this check?

@DavidBachmann
Copy link
Author

Thanks for clearing this up @giuseppeg. I second @eirikurn's proposal, though.

@giuseppeg
Copy link
Collaborator

giuseppeg commented Mar 25, 2017

Would you consider a PR for an optional babel flag that silences this error? For projects/libraries that "know better"

Probably although I think that we should implement #81, what do you think @rauchg ?

Keep in mind that as soon as you have a dynamic prop, two themes or a component is used with and without theme this would break because styles are not hashed at runtime and are deduped based on that hash no matter what is their final value.

eg.

const Button = ({children}, {theme = {primary: 'gray'}}) => (
  <button>
    {children}
    <style jsx>{`
      button { background-color: ${theme.primary} }
    `}</style>
  </button>
)

// later ...

<button>howdy</button> 

<ImaginaryThemeProvider theme={{primary: 'blue'}}>
  <button>howdy</button>
</ImaginaryThemeProvider>

@giuseppeg
Copy link
Collaborator

this will be possible when we add support for dynamic styles #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants