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

Global CSS not applied to AMP pages #10549

Closed
achrinza opened this issue Feb 16, 2020 · 42 comments
Closed

Global CSS not applied to AMP pages #10549

achrinza opened this issue Feb 16, 2020 · 42 comments
Assignees

Comments

@achrinza
Copy link

achrinza commented Feb 16, 2020

Bug report

Describe the bug

Global CSS (via _app.tsx/_app.js) are not being applied to AMP pages.

Non-AMP pages do not have this issue.

To Reproduce

Reproduction sandbox: https://github.com/achrinza/issue-sandbox-nexjs-global-css-amp

Step-by-step:

  1. Run npm init next-app
  2. Create styles.css
    body {
        color: #f00;
    }
  3. Create pages/_app.js
    import '../styles.css';
    
    export default ({ Component, pageProps }) => (
      <Component {...pageProps} />
    );
  4. Create pages/NonAmp.js page:
    const NonAmp = () => (
      <p>Hello, world!</p>
    );
    
    export default NonAmp;
  5. Create pages/Amp.js page
    export const config = { amp: true }
    const Amp = () => (
      <p>Hello, world!</p>
    );
    
    export default Amp;
  6. Run npm run dev
  7. View localhost:3000/Amp and localhost:3000/NonAmp - observe the lack of color: red on the AMP page.

Expected behavior

The AMP page should have a global CSS applied.

Screenshots

If applicable, add screenshots to help explain your problem.

System information

  • OS: Windows 10
  • Firefox 73.0
  • Version of Next.js: 9.2.1

Additional context

N/A

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels Mar 6, 2020
@MistaPidaus
Copy link

I have the same problem with TailwindCSS setup. I use the Hybrid AMP. Adding inline style style={{ background: '#000'}} on div works though.

My NextJS version: 9.2.2

@filipesmedeiros
Copy link

filipesmedeiros commented Mar 25, 2020

Wow, I've been trying to figure this one out for hours, thanks. I'll see if I can find the error and work on it and eventually do a PR

EDIT: I'm assuming this isn't solved yet, even in unstable versions, right?
Also, if someone could point me to the right place, as a start, that would be awesome.

@filipesmedeiros
Copy link

It has to be somewhere in this file https://github.com/zeit/next.js/blob/canary/packages/next/next-server/server/render.tsx right?

@filipesmedeiros
Copy link

Btw @achrinza as a temp fix, you can probably import your .css like this:

export default ({ Component, pageProps }) => <>
    <Head>
        <link rel="stylesheet" src="/styles.css" />
    </Head>
    <Component {...pageProps} />
</>

Assuming, ofc, you have the .css in the public dir. This, for some reason however, throws an Amp Validation error

[ warn ]  Amp Validation

/  error  The attribute 'href' in tag 'link rel=stylesheet for fonts' is set to the invalid value '/styles/global.css'.  https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#custom-fonts

(This error happens even if the is the only child of )

This works fine afaik.

Also, this should point in the right direction. Why does importing with import not work, while linking inside head does?
Maybe someone from the team can help.

@achrinza
Copy link
Author

achrinza commented Mar 25, 2020

@filipesmedeiros Thanks for the the workaround.

Unfortunately (based on the error), this won't generate a valid AMP page which would prevent the AMP Cache from caching the web page. It'll also breaking some CI/CD pipelines that run the AMP validator.

So unfortunately it’s a no-no for my projects

@achrinza
Copy link
Author

FWIW #8710 seems to be the PR that adds Global CSS

@filipesmedeiros
Copy link

@achrinza True, this doesn't generate valid AMP. I think for now the easiest way then, to have AMP and a "global" style would be to just inline it in the of the _app.js using styled jsx.

@filipesmedeiros
Copy link

filipesmedeiros commented Mar 25, 2020

So wait, this should be the cause right: webpack is not putting the imported stylesheets inside <style amp-custom>, maybe it has nothing to do with Next.

EDIT: nevermind, this should be something like the way Next is grabbing those global styles and passing them to AMP for the HTML generation.

@filipesmedeiros
Copy link

filipesmedeiros commented Mar 26, 2020

@timneutkens Sorry to tag you on this, but I have the feeling you can point me in the right direction (or tag someone who can?) so I can try to have a fix for this.

I've been on this for a couple of hours, and I think this has to do with the Webpack config because:

  1. styled jsx styles still get thrown into <style amp-custom="">
  2. Webpack gets all the global styles in the <head> on normal pages

So I think to solve this, we need to find a way to tell Webpack to grab the global stylesheets and put them inside <style amp-custom="">. Maybe all of this is really obvious to you guys!

So question is: can you guys point me in the direction of where we are telling Webpack what to do with global stylesheets and how to access the flag that tells us if a page is AMP or not?

If this is asking too much, let me know.

Thanks!

@timneutkens
Copy link
Member

timneutkens commented Mar 27, 2020

Webpack only compiles the stylesheet. It outputs it into a directory under .next/static/css. Main thing that has to be done is inline the full stylesheets needed under one tag in packages/next/next-server/server.tsx / packages/next/pages/document.tsx.

@filipesmedeiros
Copy link

filipesmedeiros commented Mar 27, 2020

Thanks, gonna check it out.

EDIT: Ah btw:

Webpack only compiles the stylesheet. It outputs it into a directory under .next/static/css

I assume you're talking about prod right? Because in dev, each .css file (even if @imported inside another and not globally imported on _app.js) gets its own <style> tag.

In any case, I'll look at what you said of inlining all the compiled stylesheets in one tag!

@sebastianbenz
Copy link
Contributor

Not sure if this is the best option, but one approach could be to implement a custom transformer for AMP Optimizer which collects all inline styles and merges it into <style amp-custom>. I've implemented a similar transformer for the eleventy integration.

@filipesmedeiros
Copy link

@sebastianbenz you mean after reading the files and getting then inside the markup? So:

  1. Read a global .css file
  2. Create a <style> tag for it
  3. Append the contents of that tag to the <amp-custom> one

Something like this? I was trying to read directly from the file into the <amp-custom> but maybe that would be easier? I don't know

@sebastianbenz
Copy link
Contributor

If reading from the file into amp-custom works, then it's probably the easiest way.

@filipesmedeiros
Copy link

I'm having a hard time connecting the two parts, mainly because it seems that when AMP mode is on, the dev server doesn't execute any code in this client Webpack loader, which seems to control where the global CSS ends up, at least in dev.

Tim said (for prod I assumed) we only need to append the fully compiled .css file into <amp-custom> which should be relatively easy, but I'm kinda lost in the code: don't know where to access this file the correct way. I'm trying to pass a globalStyles prop to Document's context._documentProps, in this line it will just append it before the curStyles.

So currently it seems that for prod and dev, the methods of appending CSS into <amp-custom> are kinda different.

Still exploring, though

@Timer Timer removed the help wanted label Jun 3, 2020
@apoorvmote
Copy link

Why nobody is talking about SASS support for amp? I looked through all open and closed issues. I mean there are no open issues even entertaining idea of SASS with amp. I understand it will take a while to get implemented. I would just like to have glimmer of hope that one day I will get built in SASS/SCSS support for amp.

@filipesmedeiros
Copy link

The problem is not SASS I think, it's importing CSS in general with AMP. I think AMP itself is very restrictive on how CSS in handled, so importing is weird with AMP. Best way to do it is with styled-components, emotion, etc, I think

@bulentala
Copy link

I did not have the opportunity to try it, but I saw something about it at this address.

https://www.npmjs.com/package/purify-amp-css

@apoorvmote
Copy link

apoorvmote commented Jul 10, 2020

Best way to do it is with styled-components, emotion, etc, I think

@filipesmedeiros I disagree. Main objective of AMP is not to make it easy for development but its customer experience. Also there is no custom javascript allowed in general. And they have really good reason for it. You can use it for edge cases.

@bulentala you can do all that with postCSS. Because either way you need to support for all flavour of browser e.g. mozella, safari, chrome. They all have their propriatary prefixes to css. Also most of these libraries will make your css worse than it already is.

I think Nextjs people have done fantastic job for making it easy to add AMP pages. I have been building amp websites with Hugo SSG. And nextjs does save time for adding AMP pages. It will just take longer without SASS and other stuff.

Edit:
Just saw web.dev live on youtube and using javascript (especially react) in amp is available in very small capacity. And it can only go up from here.

@timneutkens
Copy link
Member

Why nobody is talking about SASS support for amp? I looked through all open and closed issues. I mean there are no open issues even entertaining idea of SASS with amp. I understand it will take a while to get implemented. I would just like to have glimmer of hope that one day I will get built in SASS/SCSS support for amp.

Sass/CSS handling is the same so this issue covers both already. It's marked good first issue so feel free to send a PR

@apoorvmote

This comment has been minimized.

@jpbow
Copy link

jpbow commented Nov 12, 2020

I got it working for production builds in #17914 but I'm not sure what I need to tweak to get styles working in development when viewing AMP pages. Did you make any progress on this @filipesmedeiros ?

@filipesmedeiros
Copy link

@jpbow Not really sorry, I actually kinda gave up on AMP, because performance was good enough without it. Tbh I'm starting to think that having another standard (AMP) is not as productive as trying to optimize "regular" website made, for example, with Next.js or React. With this said, I know very little about AMP so I might be wrong.

@masihjahangiri
Copy link

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

@untitledlt

This comment has been minimized.

@Timer Timer removed the good first issue Easy to fix issues, good for newcomers label Nov 16, 2020
@timneutkens timneutkens added this to the iteration 15 milestone Dec 14, 2020
@ho3einmolavi

This comment has been minimized.

@KasraAhmadi

This comment has been minimized.

@Zizico2
Copy link

Zizico2 commented Jan 4, 2021

I use tailwindcss with AMP framework and need it to be fix.

For people wanting this feature because of tailwind, tailwind-in-js:
https://github.com/tw-in-js/twind

@amiralies
Copy link
Contributor

I think it's also possible to achvie this using raw-loader.

@tonny008
Copy link

tonny008 commented Mar 3, 2021

it's also possible to achvie this using styled jsx loader
next.config.js

module.exports = {
  webpack: (config, options) => {
    config.module.rules.push({
      test: /\.css$/,
      use: [
        options.defaultLoaders.babel,
        {
          loader: require('styled-jsx/webpack').loader,
          options: { type: 'global' },
        },
      ],
    });

    return config;
  },
};

and

import styles from '../styles.css';

      <style jsx global>
        {styles}
      </style>

@bulentala
Copy link

@tonny008 Warning: Built-in CSS support is being disabled due to custom CSS configuration being detected.
See here for more info: https://err.sh/next.js/built-in-css-disabled

@amorphius
Copy link

so no luck resolving this issue after 16 months?

@mikestopcontinues
Copy link

Given that Google is no longer giving priority to AMP pages, I think it makes the most sense to abandon AMP as a strategy. Our dev hours are likely better spent making our sites work well on mobile.

@amorphius
Copy link

Given that Google is no longer giving priority to AMP pages, I think it makes the most sense to abandon AMP as a strategy. Our dev hours are likely better spent making our sites work well on mobile.

Hmm, didn't know about that. Is there any good article about the reason and what is the future of AMP according to Google? So is it worth making AMP websites now or better to wait for some alternatives?

@jeremyandbandit

This comment has been minimized.

@hyoretsu
Copy link

Given that Google is no longer giving priority to AMP pages, I think it makes the most sense to abandon AMP as a strategy. Our dev hours are likely better spent making our sites work well on mobile.

Thank you and my luck of checking this issue. AMP is too... Weird, I was about to use Linaria for normal styling and Styled Components for the sole purpose of supporting AMP (aside from the 15 pages I had open about AMP-specific things)

@rawan249

This comment was marked as spam.

@rawan249

This comment was marked as spam.

@amerzad
Copy link

amerzad commented Dec 5, 2022

AMP is not about getting priority in SERP, and it was never about that except for people who are opportunists. AMP is about inventing a new evergreen HTML standard which continuously removes outdated practices and optimizing for performance automatically, and it started for mobile but it is now for every device form factor.

Many publishing companies are making AMP only responsive complete websites and they are using Next.js to ease some tasks like SSG.

Most of the people who stopped being attracted to AMP are talking about the difficulty of maintaining two websites, but I think they are very outdated about how powerful AMP library got in recent years. Actually we were able easily to do almost all the features we need for a modern publishing website using AMP only, including login and custom CAPCHA. Now, when we need to use some advanced CSR features we will use either hybrid AMP for that page/app (and possibly use AMP-to-PWA pattern), or skip the AMP completely, but the majority (if not all) are done in AMP only with amp-script and advanced usage of amp state.

This is an alternative route compared to hydration, but it is better than hydration in our use case because the page is fully interactive once it is loaded and it is smaller as there will not be hydration data payload.

After all, supporting Web Vitals is not an easy task for the average developer, and AMP is making it almost ready out of the box once the validation is complete.

@cdsimmons
Copy link

I wasn't having much luck with the solution from @tonny008 so I did another workaround.

My use case is having a single global CSS file imported in the app.js, which has my tailwind imports etc.

First compiled the imported CSS into its own file to make it available later:

const MiniCssExtractPlugin = require('mini-css-extract-plugin');

module.exports = {
  webpack: (config, options) => {
    config.plugins.push(
      new MiniCssExtractPlugin({
        filename: '../public/global.css',
      })
    );

    return config;
  },
};

Then fetch and embed for amp pages:

import '../styles/global.css';

type MyAppProps = AppProps & {
  css: string;
};

const MyApp = ({ Component, pageProps, css }: MyAppProps) => {
  return (
    <>
      {isAmp && <style jsx global>{css}</style>}
      <Component {...pageProps} />
    </>
  )
};

MyApp.getInitialProps = async () => {
  const res = await fetch('http://localhost:3000/global.css');
  const css = await res.text();
  return { css }
}

Loses the advantages of chunking and does not work for CSS modules but fits my use case.

@leerob
Copy link
Member

leerob commented Nov 8, 2023

Thank you for taking the time to open this. We are not planning to add additional features / changes to the current AMP implementation.

@leerob leerob closed this as completed Nov 8, 2023
@vercel vercel locked as resolved and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests