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

@media query interpolation broken in 12.0.4 #31562

Closed
Eyas opened this issue Nov 18, 2021 · 4 comments · Fixed by #32490
Closed

@media query interpolation broken in 12.0.4 #31562

Eyas opened this issue Nov 18, 2021 · 4 comments · Fixed by #32490
Assignees
Labels
SWC Related to minification/transpilation in Next.js.

Comments

@Eyas
Copy link

Eyas commented Nov 18, 2021

What version of Next.js are you using?

12.0.4

What version of Node.js are you using?

v16.11.0

What browser are you using?

Chrome

What operating system are you using?

Windows

How are you deploying your application?

next dev

Describe the Bug

PR #31407 fixed #30480 but introduced a new issue where interpolated strings in media queries are sometimes unnecessarily parenthesized.

Consider this, which used to work in 12.0.3 and before:

/// theme.ts

// Media Queries
export const Target: { [size in ScreenSizeQuery]: string } &
  { [prop in OtherQueries]: string } = {
  smallOnly: "screen and (max-width: 580px)",
  smallPlus: "screen and (min-width: 580px)",
  mediumPlus: "screen and (min-width: 700px)",
  largePlus: "screen and (min-width: 900px)",
  xlPlus: "screen and (min-width: 1200px)",
  touchScreen: "(hover: none), (pointer: coarse)",
};

And elsewhere in JSX:

/// _app.tsx

// ...
      <style jsx global>{`
        html {
          font-size: ${Typography.base.size.default};
          line-height: ${Typography.base.lineHeight};
        }
        @media ${Target.mediumPlus} {
          html {
            font-size: ${Typography.base.size.mediumPlus};
          }
        }
        @media ${Target.largePlus} {
          html {
            font-size: ${Typography.base.size.largePlus};
          }
        }
      `}</style>
// ...

Expected Behavior

This should compile into:

            html {
                font-size: 16px;
                line-height: 1.75;
            }

            @media screen and (min-width: 700px) {
                html {
                    font-size:18px;
                }
            }

            @media screen and (min-width: 900px) {
                html {
                    font-size:21px;
                }
            }

To Reproduce

Instead, in 12.0.4, it compiles into:

html {font-size:16px; line-height:1.75}
@media (screen and (min-width: 700px)) {html {font-size:18px}}
@media (screen and (min-width: 900px)) {html {font-size:21px}}

Notably, this should be @media screen and (min-width: 700px) {} rather than @media (screen and (min-width: 700px)) {}.

@Eyas Eyas added the bug Issue was opened via the bug report template. label Nov 18, 2021
@Eyas
Copy link
Author

Eyas commented Nov 18, 2021

cc @kdy1 who probably has the most context.

@Eyas
Copy link
Author

Eyas commented Nov 24, 2021

Specifically, the ({}) formatting here should not be happening unconditionally:

impl VisitMut for CssPlaceholderFixer {
fn visit_mut_media_query(&mut self, q: &mut MediaQuery) {
q.visit_mut_children_with(self);
match q {
MediaQuery::Ident(q) => {
if q.raw.starts_with("__styled-jsx-placeholder-") {
// TODO(kdy1): Remove this once we have CST for media query.
// We need good error recovery for media queries to handle this.
q.raw = format!("({})", &q.value).into();
}
}
_ => {}
}
}
}

@timneutkens timneutkens added SWC Related to minification/transpilation in Next.js. kind: bug and removed bug Issue was opened via the bug report template. labels Nov 26, 2021
@Eyas
Copy link
Author

Eyas commented Dec 10, 2021

Hey all -- just checking if there's any update on this. I've seen improvements around styled_jsx swc transforms but none seem to address this issue. It's blocking a few folks from upgrading to 12.0.4 1.

Footnotes

  1. (or 12.* at all, since each of the previous patch versions before this logic was introduced had their own upgrade blocking issues..)

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
SWC Related to minification/transpilation in Next.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants