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

make static css immutable #5675

Closed
wants to merge 1 commit into from
Closed

make static css immutable #5675

wants to merge 1 commit into from

Conversation

Enalmada
Copy link
Contributor

@Enalmada Enalmada commented Nov 14, 2018

css is currently max-age 0 which means it won't be cached by cdn. This fixes that along with

Related: vercel/next-plugins#335

Fixes #5464
Fixes vercel/next-plugins#243

@kachkaev
Copy link
Contributor

Also related to vercel/next-plugins#243

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong button.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed when we have vercel/next-plugins#335 🤔

@Enalmada
Copy link
Contributor Author

I started with the other (#335) and it fixed some but not all the css max-age=0. So I hunted down what was adding immutable and it seems like there are two different type of static css and this gets the other one. I tried clearing my browser cache, .next, node_modules/.cache to be sure...it couldn't hurt for someone to double check though.

@timneutkens
Copy link
Member

I don't think we should put the implementation detail of a Next.js plugin inside of the Next.js core, especially when it doesn't add anything in this case because you've fixed the root cause of the issue.

@Enalmada
Copy link
Contributor Author

Enalmada commented Nov 14, 2018

Well, I only added this when the other thing didn't seem to work completely...it did help with some of the css...just not all...seems like some css start with "/_next/static/css" and not "/_next/static/chunks" despite the plugins change. Could that be from styled jsx or something that isn't "next-css" plugins css?

"/static/chunks" is now working immutable...
image

but commons and page specific css for pages requires this change or else it isn't immutable
image

But perhaps there is a better way to fix this? Is there anything under /_next/static that shouldn't be immutable? (feels like the fix should just be to make everything under that immutable rather than look for "chunks" but perhaps there is some reason for that.

@timneutkens
Copy link
Member

timneutkens commented Nov 15, 2018

but commons and page specific css for pages requires this change or else it isn't immutable

This doesn't exist in next-css. next-css only bundles everything into 1 file.

@Enalmada
Copy link
Contributor Author

Enalmada commented Nov 15, 2018

Interesting. So my pages are following the "with-reasonml" example and my reason-react pages call bindings like this that require antd less/css from node_modules like this:
[%bs.raw {|require("antd/lib/menu/style")|}];
Perhaps that require is making Next.js/webpack chunk things in unexpected ways that don't go through next-css?

Ultimately the ant design styles that get required for each of my pages ends up being served maxage=0 by next.js in a chunk for each page like this: .next/static/css/static\qfHVEQIBeZTs5GFjgVDub\pages\index.js.b2aa1ebf.chunk.css. Everything served out of "BUILD_ID/pages" by nextjs could/should be immutable though right? Perhaps this fix is appropriate then to handle this use case?

@Enalmada
Copy link
Contributor Author

Example reproduction here: https://github.com/Enalmada/next-reason-boilerplate
(Note that the custom server is working around the issue by adding immutable to all next/static/css)

I am using ant design and their babel-plugin-import. I am under the impression that is just automatically adding requires for css I need and I assumed next-css would handle it from there but perhaps not, perhaps require calls skip next-css. The css still ends up being served by nextjs so I think this fix is the only way to handle it.

@Enalmada
Copy link
Contributor Author

Can we merge this? It is definitely necessary to make the css files directly required by reasonml files immutable and I would love to get this fixed unless there is a better way to do that.

@timneutkens
Copy link
Member

Asked @kylemh and @jhoffmcd to have a look at this to make sure this is the correct fix 👍

@jhoffmcd
Copy link
Contributor

@Enalmada just to catch me up, we think that there is an interop issue with the ReasonML integration that is causing the undesirable output of the css bundle under static/css, and the fix is to include css string check in the immutable assignment. Does this sound about right? Just getting up to speed on the thread so far.

@Enalmada
Copy link
Contributor Author

Enalmada commented Dec 11, 2018

@jhoffmcd Thanks for taking a look at this! I am pretty sure the problem stems from either the way 3rd party reasonml bindings requires css files: example here, or the way babel-plugin-import auto-imports css which is usually used in conjunction with ant design. I did my best to fix but it is possible this fix could be implemented better/simpler. I feel like the whole static directory perhaps should always be immutable...I am unsure a chain of checks for different things is really necessary....why not just make everything from that directory always immutable?

@jhoffmcd
Copy link
Contributor

@Enalmada
Copy link
Contributor Author

@jhoffmcd I updated the example repo with next-css:1.0.2-canary.2 and when running in production mode, it unfortunately only fixes one of 5 css files:
image

4 css files are still being served out of "/_next/static/css" rather than "/_next/static/chunks" and thus missing the fix in 1.0.2-canary.2. The mystery is what is the right way to get immutable on the rest of them. I am assuming the way they are being loaded is skipping next-css and a fix like this pull request is necessary but perhaps that assumption is wrong and they should end up being processed by next-css in a way I don't yet understand how to enable.

@jhoffmcd
Copy link
Contributor

@Enalmada Is is possible to get a reproduction with a smaller sub set of dependancies? The boilerplate you have set up integrates a large number of libraries and tooling and I would like to understand the source of the problem before we modify core behavior.

Running canary versions of next and next-plugins alone, I cannot reproduce this outcome. If you think its antd and babel-plugin-import lets try a minimal version with those things and a custom server implementation and see if we get the same result.

@Enalmada
Copy link
Contributor Author

Ok I take it back with egg on my face...you are right! Earlier I updated @zeit/next-css but I now notice that there is a @zeit/next-less in my dependencies I forgot to update from 1.0.1. When i upgrade that to 1.0.2-canary.2 I can't reproduce the problem anymore. So something in the canary did indeed fix this and all plugins need update rather than just next-css. Thank you so much for the time you put into trying to reproduce this.

@Enalmada Enalmada closed this Dec 11, 2018
@jhoffmcd
Copy link
Contributor

Awesome! No problem @Enalmada I'm glad you found the source!

@lock lock bot locked as resolved and limited conversation to collaborators Dec 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

static css chunks served with maxage=0 next-css does not apply max-age=31536000 to /_next/static/css
4 participants