search

vercel / next.js

The React Framework
https://nextjs.org
MIT License
124.97k stars 26.69k forks source link

AMP: styles missing when importing standard css files in a project using styled components #7121

Closed wouterds closed 2 months ago

wouterds commented 5 years ago

Bug report

Describe the bug

When importing standard css files from node modules, e.g.:

import 'reset-css/reset.css';
import 'semantic-ui-css/components/form.min.css';
import 'semantic-ui-css/components/message.min.css';

and when you're using styled components, only the styled components remain. The styles of the normal css files imported from node modules disappear / are missing (in the AMP version).

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Use styled components in your project
  2. Import css from a lib that doesn't use styled components, e.g. css reset (import 'reset-css/reset.css';)
  3. Use withAmp on your page
  4. View the amp version and notice standard imported css is missing

Expected behavior

Import both styled components and other imported css from files.

System information

Additional context

Styled components seem to be working as of this version for AMP, but other css next to styled components is being thrown away in the AMP version.

ijjk commented 5 years ago

Hi, you can use the styled-jsx Webpack loader to import external CSS in AMP mode for now. You just have to be careful to not go over the 50,000 bytes limit for styles in AMP.

wouterds commented 5 years ago

@ijjk Could you illustrate with an example how that's supposed to work? Because this doesn't.

{isAmp && (
  <style jsx global>{`
    @import '~reset-css/reset.css';
    @import '~semantic-ui-css/components/form.min.css';
    @import '~semantic-ui-css/components/message.min.css';

    body {
      background: red;
    }
  `}</style>
)}

Edit: to clarify; background: red; works, but the external stylesheets are not loaded.

ijjk commented 5 years ago

@wouterds I was mistaken, we don't support that in AMP mode right now (sorry about that), currently to do this you would need to load the styles in a custom _document. Note: this is not ideal and is just a workaround you can use while official handling of this is implemented.

Example with custom pages/_document:

import Document, { Html, Head, Main, NextScript } from 'next/document'
import fs from 'fs'
import path from 'path'
import { promisify } from 'util'

const readFile = promisify(fs.readFile)

class MyDocument extends Document {
  static async getInitialProps(ctx) {
    const initialProps = await Document.getInitialProps(ctx)
    // assumes .next is your distDir
    const projectDir = __dirname.split('.next')[0]
    const customStyles = await readFile(
      // to load normalize from node_modules change
      // styles.css to node_modules/normalize.css/normalize.css
      path.join(projectDir, 'styles.css'),
      'utf8'
    )

    return {
      ...initialProps,
      styles: (
        <>
          <style dangerouslySetInnerHTML={{
            __html: customStyles
          }} />
          {initialProps.styles}
        </>
      )
    }
  }

  render() {
    return (
      <Html>
        <Head />
        <body>
          <Main />
          <NextScript />
        </body>
      </Html>
    )
  }
}

export default MyDocument
samohovets commented 5 years ago

I don't want to bother anyone, but any progress on this, guys?

koenverburg commented 5 years ago

@ijjk how would you do it when you have app/pages instead of <rootdir>/pages?

ijjk commented 5 years ago

@koenverburg it should work the same, are you getting an error using the above workaround?

maraisr commented 5 years ago

@timneutkens any clue on an ETA to actually support AMP within this project?

timneutkens commented 5 years ago

Will probably be part of https://github.com/zeit/next.js/issues/8626

Haven't prioritized this issue in particular, there are more important ones on the roadmap.

It's unfair to say "actually support", Next.js supports AMP out of the box, you're adding in non-standard behavior (css imports) and linking css files is not even supported in AMP, they'd have to be inlined also.

maraisr commented 5 years ago

So you know how when you run a import './my-styles.css' - why does next link that on browser runtimes, but gets completely ignored for amp builds? Why cant next just inline that import, much like its linking it now? And yes, but "actually supporting" I mean exactly that. A export const config = { amp: 'hybrid' }, to actually be supported - css imports that are currently linked, should be inlined.

But thanks for clarifying @timneutkens - this tool almost works like a charm 💃

timneutkens commented 5 years ago

I'm not sure what else to discuss here. As said Next.js supports AMP and you can't reason that it doesn't. You're reasoning "AMP doesn't work in my particular edge case".

As said it might be part of #8626.

Note that there are better solutions than css imports for creating AMP pages, like using styled-jsx / other css-in-js libraries. AMP has restrictions on the amount of css that can be sent which are very restrictive so css imports will send too much css in general.

eglove commented 4 years ago

Styled JSX throws amp-validation errors.

The mandatory attribute 'amp-custom' is missing in tag 'style amp-custom (transformed)'.

When using <style amp-custom>{'...'}</style> in <Head> (as it is supposed to be) it compiles in HTML as: <style amp-custom content=""></style><style>(styled JSX here)</style>

That's why this is a problem.

Tzelon commented 4 years ago

8626 didn't resolve this issue for me. And the workaround failed on zeit now.

Is there a way to make this work? Thank you

pavelzubov commented 4 years ago

8626 didn't resolve this issue for me either.

geryit commented 4 years ago

any update on this?

<Head>
  <title>{title}</title>
  <style amp-custom>{`body {background-color: gray;}`}</style>
</Head>

is compiled as (in prod): 

<head>
  <style>body {background-color: gray;}</style>
  <link rel="canonical" href="/">
  <style amp-custom></style>
</head>
Screen Shot 2020-02-19 at 11 52 50 AM
selodev commented 4 years ago

Can you please add this future for all css, scss and css, scss modules import to work with AMP. I have wasted 3 months now. FML

timneutkens commented 4 years ago

@selahattintoprak feel free to send a PR to add it

geekplux commented 4 years ago

hello folks, I found a solution:

I used tailwindcss with AMP as an example and exposed completed code in my post https://dev.to/geekplux/how-to-use-tailwindcss-with-amp-in-a-next-js-project-1f97

cjimmy commented 4 years ago

☝️ @geekplux's workaround worked well for me. I am not using tailwindcss, but am using styled-components, and wanted to use a global css file. To expand a bit on what it entails:

Differences in my implementation:

Downside of this implementation:

joelcoluccinbc commented 4 years ago

Another workaround is to leverage the styled-css global feature + Next.js useAmp hook. See blog here: https://nextjs.org/blog/styling-next-with-styled-jsx#writing-styles-in-external-files

Example usage:

import React from 'react';
import { useAmp } from 'next/amp';

import globalStyles from '../styles/global.js';

function DefaultLayout () {
  const isAmp = useAmp();
  return (
    <div>
      {isAmp && <style jsx global>
        {globalStyles}
      </style>
      }
    </div>
  );
}

Downside is copying versioned files to your project.

masihjahangiri commented 3 years ago

It's a really big issue because I use tailwind and I need style AMP pages with tailwindcss classes.

codercatdev commented 3 years ago

Whew glad I found this issue I have been trying to dig into the global.css not importing for an hour. Looks like Tim added the good first issue 🏷️ so I will dive into it some more. Anyone already working on it??

ibraelillo commented 3 years ago

@hi guys, let me give you my wisdom in this issue :)

i've working for some days now on a project where I was forced to do an amp version, using @tailwindcss. This is what i've done and it works, really elegant by the way.

// .babelrc

{
    "presets": [
        [
            "next/babel",
            {
                "styled-jsx": {
                    "plugins": [
                        "styled-jsx-plugin-postcss"
                    ]
                }
            }
        ]
    ]
}

// package.json

{
"dependencies": {
    "classnames": "^2.2.6",
    "next": "^10.0.6",
    "next-pwa": "^5.0.5",
    "react": "^17.0.1",
    "react-dom": "^17.0.1",
    "tailwindcss": "^2.0.2"
  },
  "devDependencies": {
    "babel-eslint": "^10.1.0",
    "eslint": "^7.19.0",
    "eslint-config-prettier": "^7.2.0",
    "eslint-loader": "^4.0.2",
    "eslint-plugin-prettier": "^3.3.1",
    "eslint-plugin-react": "^7.22.0",
    "postcss": "8.2.4",
    "postcss-flexbugs-fixes": "^5.0.2",
    "postcss-import": "^14.0.0",
    "postcss-preset-env": "^6.7.0",
    "prettier": "2.0.5",
    "styled-jsx-plugin-postcss": "4.0.1"
  }
}

// any component, this is just an real example from my current code

import Image from 'components/Image'
import React from 'react'

export const Pilot = ({ name, image, job }) => {
  return (
    <>
      <div className={'root'}>
        <div className={'card'}>
          <div className={'imgContainer'}>
            <Image
              layout="responsive"
              width={100}
              height={90}
              alt={name}
              src={image}
            />
          </div>
          <div className={'name'}>{name}</div>
          <div className={'job'}>{job}</div>
        </div>
      </div>
      <style jsx>{`
        .root {
          @apply w-full;
        }

        .card {
          @apply bg-white shadow rounded-lg p-1 text-center h-auto;
        }

        .imgContainer {
          @apply p-4 relative h-full;
        }

        .name {
          @apply font-semibold;
        }

        .job {
          @apply text-xs text-gray-700;
        }
      `}</style>
    </>
  )
}

As you will suppose now, the styled-jsx-postcss-plugin delegate to postcss the css transformation, using the same postcss config that you should use inside postcss.config.js .

module.exports = {
  plugins: {
    'postcss-import': {},
    tailwindcss: require('./tailwind.config'),
    'postcss-custom-properties': {},
    'postcss-flexbugs-fixes': {},
    'postcss-preset-env': {
      autoprefixer: {
        flexbox: 'no-2009',
      },
      stage: 3,
      features: {
        'custom-properties': true,
      },
    },
  },
}

Honestly, this setup took me 15 minutes to make it work, and it works like a charm. Only non-positive point to signal is that in some cases the styled-jsx-plugin-postcss shutdown, because of some memory leaks I guess, but that's only a small issue.

Hope this helps some of you to achieve your work.

masihjahangiri commented 3 years ago

@Timer I'm surprised by the strong next.js team that they don't take any step to fix this bug. Personally, I would like to contribute and fix this bug, but I'm sure you will not accept my PR.

Jorenm commented 3 years ago

This was my solution to the issue. It only works for the built site which is annoying, but isn't unworkable. I just set up my vercel build command to be "next build && node postprocess.js"


const dirTree = require("directory-tree")

const css = dirTree('./.next/static/css')

const fonts = `@font-face {
    font-family: 'BB Roller Mono';
    font-style: normal;
    font-weight: normal;
    src: url('/fonts/BB Roller Mono/bbrollermonoprotx-regular.otf') format('opentype');
}
body {font-family: Optimo-Plain}
`

// Concatenate all generated css
let styles = ''
css.children.forEach((file) => {
    styles += fs.readFileSync(file.path)
})

// Amp doesn't like !important
styles = styles.replace('!important', '')

const ampPages = dirTree('./.next/server/pages/amp')

// Loop over a page and extract all the classnames. Might need to loop over more than one depending on how unique they are.
let preservedClassnames = []
for (let i=0; i<ampPages.children.length; i++) {
    const folder = ampPages.children[i]
    if (folder.children) {
        const childFile = folder.children[0]
        if (childFile.extension == '.html') {
            let html = fs.readFileSync(childFile.path).toString()
            var regex = /class="([^"]+?)"/g;

            let matches = []
            while (matches = regex.exec(html)) {
                preservedClassnames.push(matches[1])
            }

            break;
        }
    }
}

let filteredStyles = ''

// Extract the css rules for the extracted class names.
preservedClassnames.forEach((classNames) => {
    classNames.split(' ').forEach((className) => {
        const styleRegex = new RegExp(`\.${className}.*?{[^}]+}`, 'g')
        const matches = styles.match(styleRegex)
        if (matches) {
            filteredStyles += matches.join('')
        }
    })
})

// Add some needed fonts.
filteredStyles = fonts + filteredStyles

// Populate the amp-custom tag with the styles.
ampPages.children.forEach((file) => {
    if (file.children) {
        file.children.forEach((childFile) => {
            if (childFile.extension == '.html') {
                let html = fs.readFileSync(childFile.path).toString()

                html = html.replace('<style amp-custom></style>', `<style amp-custom>${filteredStyles}</style>`)
                console.log(`Adding amp styles to ${childFile.path}`)
                fs.writeFileSync(childFile.path, html)
            }
        })
    }
})

console.log('POST PROCESSED')
github-actions[bot] commented 2 months ago

We are in the process of closing issues dating from 2019-01-01 to 2019-12-31 to improve our focus on the most relevant and actionable problems.

Why are we doing this?

Stale issues often lack recent updates and clear reproductions, making them difficult to address effectively. Our objective is to prioritize the most upvoted and actionable issues that have up-to-date reproductions, enabling us to resolve bugs more efficiently.

Why these issues?

Issues dating from 2019-01-01 to 2019-12-31 are likely to be outdated and less relevant to the current state of the codebase. By closing these older stale issues, we can better focus our efforts on more recent and relevant problems, ensuring a more effective and streamlined workflow.

If your issue is still relevant, please reopen it using our bug report template. Be sure to include any important context from the original issue in your new report.

Thank you for your understanding and contributions.

Best regards, The Next.js Team

github-actions[bot] commented 2 months ago

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.