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

Topics/regroup non static files #6346

Merged
merged 18 commits into from
Jul 17, 2018
Merged

Topics/regroup non static files #6346

merged 18 commits into from
Jul 17, 2018

Conversation

brotzky
Copy link
Contributor

@brotzky brotzky commented Jul 8, 2018

Continuation of #6285

  • Removed --group-files flag and made grouping default
  • Cleaned up rebase problem

/public/ output

screen shot 2018-07-08 at 3 42 04 pm

index.html with no prefix

screen shot 2018-07-08 at 3 41 53 pm

index.html with prefix of /blog

screen shot 2018-07-08 at 3 40 53 pm

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 8, 2018

Deploy preview for using-drupal ready!

Built with commit f1ad758

https://deploy-preview-6346--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 8, 2018

Deploy preview for gatsbygram ready!

Built with commit f1ad758

https://deploy-preview-6346--gatsbygram.netlify.com

@@ -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`,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is ../css/ needed?

Copy link
Contributor Author

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.

@grega
Copy link

grega commented Jul 11, 2018

Have tested this and it's looking great - a big benefit for us, thank you!

@Jonic
Copy link

Jonic commented Jul 12, 2018

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"
Copy link
Contributor

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

@m-allanson
Copy link
Contributor

This looks great, thanks @brotzky! I added one nitpicky comment, then I think this is ready to merge.

@@ -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`,
Copy link
Contributor Author

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
Copy link
Contributor Author

@brotzky brotzky Jul 12, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 👍

@m-allanson
Copy link
Contributor

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 src/assets/. They're imported in the Layout component.

Once the site is built they end up in public/js/static/ instead of public/static:

ls public/js/static/
inter-ui-bold-a20157ae226e82804c971f33890925f7.woff2
inter-ui-bold-bfbca0680dd1ae394462ba9649e3c06d.woff
inter-ui-italic-0dbe24780396c3843fb2f3fc536761b1.woff2
inter-ui-italic-4f71078805ff7dba8db988b54caaf06a.woff
inter-ui-regular-5f4e2851ddf0dcdcb56ce5f4c4f13758.woff
inter-ui-regular-73a04e2380ff2e8195f3c67807de9b2a.woff2

@brotzky
Copy link
Contributor Author

brotzky commented Jul 12, 2018

@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/`
Copy link
Contributor Author

@brotzky brotzky Jul 13, 2018

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

screen shot 2018-07-12 at 7 00 45 pm

Copy link
Contributor

@m-allanson m-allanson left a 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.

@m-allanson m-allanson merged commit e401237 into gatsbyjs:master Jul 17, 2018
@gatsbot
Copy link

gatsbot bot commented Jul 17, 2018

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:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

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!

@m-allanson
Copy link
Contributor

Published as gatsby@2.0.0-beta.41

@brotzky
Copy link
Contributor Author

brotzky commented Jul 17, 2018

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.

@m-allanson
Copy link
Contributor

please tag me if you think an issue is filed related to this

Will do, thank you!

@m-allanson
Copy link
Contributor

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):

5:31:55 PM: error Building static HTML for pages failed
5:31:55 PM: See our docs page on debugging HTML builds for help https://goo.gl/yL9lND
5:31:55 PM: 
5:31:55 PM:   WebpackError: ENOENT: no such file or directory, open '/opt/build/repo/www/css  /0.22e0a7306140b3769866.css'
5:31:55 PM:   
5:31:55 PM:   
5:31:55 PM:   - static-entry.js:62 Object.fs.readFileSync
5:31:55 PM:     lib/.cache/static-entry.js:62:3
5:31:55 PM:   
5:31:55 PM:   - static-entry.js:267 styles.slice.reverse.forEach.style
5:31:55 PM:     lib/.cache/static-entry.js:267:26
5:31:55 PM:   
5:31:55 PM:   

@pieh noticed that public is missing from the filepath in this line 5:31:55 PM: WebpackError: ENOENT: no such file or directory, open '/opt/build/repo/www/css /0.22e0a7306140b3769866.css', which makes me wonder if it's related to this issue?

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!

@jquense
Copy link
Contributor

jquense commented Jul 19, 2018

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

@KyleAMathews
Copy link
Contributor

@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.

KyleAMathews added a commit that referenced this pull request Jul 20, 2018
@KyleAMathews
Copy link
Contributor

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.

@iwilsonq
Copy link
Contributor

@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?

@KyleAMathews
Copy link
Contributor

@iwilsonq no plans — see @jquense's comment #6346 (comment)

Perhaps in your case you can set cookies that the load balancer can use.

@koga73
Copy link

koga73 commented Jul 10, 2019

Any update on this? Is there currently a way to output css/js into folders in /public?

@koga73
Copy link

koga73 commented Jul 16, 2019

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/

@sonusynup
Copy link

sonusynup commented Jul 24, 2019

@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?
Screenshot 2019-07-24 at 11 23 52 AM

@plasticine
Copy link

plasticine commented Jun 16, 2020

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);

/static, /js, /css  -> CDN / backend bucket
everything else     -> Webserver that can add headers (HSTS, CSP, etc)

This really sticks out to me;

@KyleAMathews — 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.

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.

🤙🏼 Edit: Also, I realise that this is a ~year old, closed issue at this point, so I apologise if bumping it up again is perceived as unhelpful. However this seems like such a glaring flaw in what is otherwise a great tool, and I’d love to see this discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.