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

Fix nested style tags break compilation #260

Merged
merged 4 commits into from
Dec 21, 2017
Merged

Conversation

giuseppeg
Copy link
Collaborator

@giuseppeg giuseppeg commented Jul 8, 2017

iirc we don't allow nested style tags like:

<div>
  <span>
      <style jsx>{...}</style>
  </span>
</div>

With this PR we ignore them because otherwise compilation breaks when there is one.

@giuseppeg giuseppeg requested review from rauchg and nkzawa July 8, 2017 07:20
@giuseppeg
Copy link
Collaborator Author

@rauchg should we instead throw an error when we detect nested style tags?

@nkzawa
Copy link
Contributor

nkzawa commented Jul 11, 2017

I think we should support this since scoped style tag works like that (as far as I know).

@giuseppeg
Copy link
Collaborator Author

giuseppeg commented Jul 11, 2017

@nkzawa for the record I found the issue #42

@giuseppeg
Copy link
Collaborator Author

@nkzawa , @rauchg can I merge this?

@rauchg
Copy link
Member

rauchg commented Aug 5, 2017

I don't think ignoring is the right solution. Throwing an error until we fully support it would be ideal

@a-ignatov-parc
Copy link
Contributor

Any news on this PR? It's been a while since last activity :)

@giuseppeg giuseppeg force-pushed the fix-nested-style-tags branch from 7612a07 to f7e7fb5 Compare December 19, 2017 20:44
@giuseppeg giuseppeg merged commit e30d2a7 into master Dec 21, 2017
@giuseppeg giuseppeg deleted the fix-nested-style-tags branch December 21, 2017 09:03
@andrechalella
Copy link

Hi, first of all thanks for the great product!

I just lost a couple hours of work (and I'm on a tight schedule) because Next.js threw up with a cryptic error when I made a simple nested style.

I'd been searching the web for more than 1 hour when I finally understood (from several issues/bug reports) that styled-jsx does not allow this kind of nesting.

It was frustrating to learn that, instead of throwing early and clearly, the team decided to simply ignore invalid code.

With this PR we ignore them because otherwise compilation breaks when there is one.

It seems obvious to me that compilation should not succeed when there is invalid code.

Could someone explain the rationale behind this? Maybe no one thought that users might be confused, was that the case? In case there's no good explanation, maybe this decision could be reversed, so styled-jsx would throw with a clear message (as per @rauchg's comment)?

Also, nowhere in the docs I couldn't find that we weren't supposed to do such nesting. Maybe that needs fixing too?


I'll leave some details below for people that might run into the same problem as I did.

Error:

wait  - compiling...
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', crates/styled_jsx/src/lib.rs:531:44
error - ./pages/test.js
Error: failed to process

pages/test.js:

export default function Home() {
  return (
    <div>
      blue
      <p>
        red
        <style jsx>{` p { color: red; } `}</style>
      </p>
      <style jsx>{` div { color: blue; } `}</style>
    </div>
  )
}

I was able to solve it with resolve. I don't like defining fixed class names, because they might clash with the global styles.

import css from 'styled-jsx/css';

const { className, styles } = css.resolve`
  p { color: red }                                                                                  
`;

export default function Home() {
  return (
    <div>
      blue
      <p className={className}>red</p>
      <style jsx>{` div { color: blue; } `}</style>
      {styles}
    </div>
  )
}

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

Successfully merging this pull request may close these issues.

5 participants