-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Topics/regroup non static files #6346
Topics/regroup non static files #6346
Conversation
…tyles/script paths.
…tyles/script paths.
…/gatsby into topics/regroup-non-static-files
Deploy preview for using-drupal ready! Built with commit f1ad758 |
Deploy preview for gatsbygram ready! Built with commit f1ad758 |
@@ -430,8 +430,8 @@ module.exports = async ({ | |||
*/ | |||
plugins.extractText = options => | |||
new MiniCssExtractPlugin({ | |||
filename: `[name].[contenthash].css`, | |||
chunkFilename: `[name].[contenthash].css`, | |||
filename: `../css/[name].[contenthash].css`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is ../css/
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the css is generated in the stage build-javascript
using the MiniCssExtractPlugin, which grabs the output path from webpack's config webpackConfig.output.path
.
From what I could tell, there's no way of changing the output path (only publicPath?) using MiniCssExtractPlugin
so adjusting the filenames is a workaround.
What's happening is the system things it's putting the CSS into the /js
folder, but it actually goes back into the root, and creates a /css
folder to place the CSS files in.
If there's a different way to handle this, such as creating build-css
or a different webpackConfig.output.path
during the time of CSS generation that would solve this workaround.
Have tested this and it's looking great - a big benefit for us, thank you! |
This is looking like a great update! It's just what we need to get this one thing shipped :) |
/> | ||
) | ||
} else { | ||
headComponents.unshift( | ||
<style | ||
data-href={urlJoin(pathPrefix, style.name)} | ||
type="text/css" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this line? See #6384
This looks great, thanks @brotzky! I added one nitpicky comment, then I think this is ready to merge. |
…/gatsby into topics/regroup-non-static-files
…changes by mistkae
@@ -430,8 +430,8 @@ module.exports = async ({ | |||
*/ | |||
plugins.extractText = options => | |||
new MiniCssExtractPlugin({ | |||
filename: `../css/[name].[contenthash].css`, | |||
chunkFilename: `../css/[name].[contenthash].css`, | |||
filename: `[name].[contenthash].css`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added back the ../css/
back as this wasn't supposed to be in this commit. See the following commit for reference.
@@ -260,7 +260,6 @@ export default (pagePath, callback) => { | |||
} else { | |||
headComponents.unshift( | |||
<style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6346 (comment) @m-allanson removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
Trying this out on a local project and it's looking good! However I noticed that assets don't always end up in the right place. Here's a demo repo: https://github.com/m-allanson/gatsby-pr-6346-demo. You'll need to run it using the version of Gatsby in this PR. I have some font files in Once the site is built they end up in
|
@m-allanson great job testing it out locally and thanks! I'll look into that bug today. Looks like an issue with file extension types not being filtered properly. |
* "build-javascript" output path of `/js`. The same technique is | ||
* used for CSS | ||
*/ | ||
const assetRelativeRoot = `../static/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-allanson This had to be treated the same way as the ../css/
case because of how there's only a build-javascript
stage. #6346 (comment)
Here are all the build stages:
type Stage = "develop" | "develop-html" | "build-javascript" | "build-html"
I think in the future it could be a good idea to add discrete build stages for CSS (although CSS in JS makes this a question mark?) and static assets instead of relying on this current workaround.
Here's an example of the output folder of your example repository with this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @brotzky 👍 let's get this merged and released.
Holy buckets, @brotzky — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
Published as |
Awesome, thanks! @m-allanson I have a feeling this feature can cause some breaks. Ill be scanning the issues going forward and please tag me if you think an issue is filed related to this. |
Will do, thank you! |
Some netlify builds are failing since this was merged. I've not been able to replicate the failure locally though. Wondering if you have any insights on this @brotzky? Here's an example failure when building next.gatsbyjs.org, from this repos www directory (full build log):
@pieh noticed that It seems like this is the line that's failling https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/static-entry.js#L270. Any ideas on what might be causing this? No worries if not! |
ya, its not straightforward to organize the webpack output, all the loaders that provide names need to specify their names relative to their new output. see: https://github.com/jquense/webpack-atoms/blob/master/src/index.js#L257 IMO opinion its not worth the extra complexity for more organized output :/ to many odd things can break |
@jquense hmmm yeah — makes sense. I'm not really sure this is worth it. Gatsby is a website compiler and its output isn't meant to be "understood" per se as you don't ever interact with it directly. |
This reverts commit e401237.
I reverted this as it's causing bugs. @brotzky this would be nice to have in but looks like there's more work that'd have to be done to make it super solid. |
@KyleAMathews since this was reverted, I'm wondering if there will be any plans to implement something to group up non statics? For example, my use case has two webapps (one gatsby, one react app) running on the same subdomain. We have a load balancer that listens for particular paths and tries to deliver the correct js bundle. While we can deliver our regular react app's bundle, the gatsby output js files are, as they stand in the root of the public folder, difficult to configure listeners for. Currently we're using the gatsby output in an nginx container hosted on aws ec2, but the issue is the same for putting it on S3, since we have to configure the paths for CloudFront to deliver the right bundles too. Any ideas? |
@iwilsonq no plans — see @jquense's comment #6346 (comment) Perhaps in your case you can set cookies that the load balancer can use. |
Any update on this? Is there currently a way to output css/js into folders in /public? |
I created a plugin for this: gatsby-plugin-tidy. It puts js/css into folders with option to name them and does some other things that makes the build output a bit nicer: https://www.gatsbyjs.org/packages/gatsby-plugin-tidy/ |
@KyleAMathews , Still I am not able to make directories for js/css files on production build. Should I update my gatsby core to avail this feature? |
Heya all 👋 I’m really hopeful that Gatsby changes their approach to this and revisits this functionality. I think the lack of partitioning of generated, static content is presents some pretty glaring issues and incompatibilities with webservers and reverse proxies in general. By way of a concrete use-case — the lack of ability to prefix paths for generated media for a site build makes it nearly impossible to effectively serve a site where complex rules around serving are required for different types of content, as most reverse proxying load balancers (for example the GCP GLB) can only operate on path prefix. For example, even for a static site, It’s a pretty common use case to want to add additional headers to page/html responses, but delegate serving any static media from a CDN for example. Currently doing this is a nightmare given how Gatsby has elected to write it’s output. To implement the above use-case, ideally you’d do the following (pseudo loadbalancer / routing config);
This really sticks out to me;
I can’t disagree more with this stance — you absolutely need to care about the generated output of a tool like Gatsby, and you absolutely need to interact with the output directly. The output is the value, it’s what you’re serving, and Gatsby doesn’t exist in a vaccum — it needs to integrate well with other tools that will naturally exist around it, and it’s unreasonable to expect them not to need to care about the structure of Gatsby’s output.
|
Continuation of #6285
--group-files
flag and made grouping default/public/
outputindex.html
with no prefixindex.html
with prefix of/blog