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

SLB-201: remove copy operations and streamline artefacts #129

Merged
merged 15 commits into from
Jan 24, 2024

Conversation

pmelab
Copy link
Contributor

@pmelab pmelab commented Jan 22, 2024

Description of changes

  • extract static file handling in Gatsby into @amazeelabs/gatsby-plugin-static-dirs (to be moved to silverback-mono after review)
  • rename Drupal stylesheets to make them easier to understand
  • remove alls cases where prep steps generate files outside of the current page, and make the consuming packages load them via node_modules instead.

Motivation and context

  • Reduce development hiccups when forgetting to re-run prep in another package to copy the right files.
  • Reduce problems with Turborepo caches (🤞)
  • Easier understanding where some files come from.

How has this been tested?

  • Manually
  • Unit tests
  • Integration tests

@pmelab pmelab force-pushed the SLB-201-reduce-copying branch from 76d946e to 8a02150 Compare January 22, 2024 19:03
@pmelab pmelab force-pushed the SLB-201-reduce-copying branch from 1784b11 to 972dc30 Compare January 23, 2024 13:39
@pmelab pmelab requested a review from Leksat January 23, 2024 14:08
@pmelab pmelab changed the title refactor(SLB-201): directly load stylesheets in gatsby SLB-201: remove copy operations and streamline artefacts Jan 23, 2024
@pmelab pmelab marked this pull request as ready for review January 23, 2024 14:10
Copy link
Member

@Leksat Leksat left a comment

Choose a reason for hiding this comment

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

I'd do something with the css browser cache issue.

The rest looks awesome!

Also, could you assign it to me for a manual testing once it is deployed?

packages/gatsby-plugin-static-dirs/gatsby-node.js Outdated Show resolved Hide resolved
apps/website/gatsby-ssr.ts Outdated Show resolved Hide resolved
Otherwise we loose proper cache handling by webpack.
To avoid a warning in Gatsby, which runs the file trough PostCSS again.
It shouldn't do that, but there is no way to stop it.
Gatsby's style handling assumes every component has its own stylesheet,
and it would add only the required styles to each page. With Tailwind though,
we have only one global stylesheet, and adding it inline everywhere achieves
the opposite.
@pmelab pmelab requested a review from Leksat January 24, 2024 10:31
Copy link
Member

@Leksat Leksat left a comment

Choose a reason for hiding this comment

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

Looks good! Let's test on Prod 😁

@@ -1,3 +1,5 @@
import './node_modules/@custom/ui/build/styles.css';
Copy link
Member

Choose a reason for hiding this comment

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

I saw that previously we imported it from both gatsby-browser and gatsby-ssr. Now it's gatsby-browser only. Not a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a mistake before.

@@ -38,6 +38,7 @@ export default {
siteUrl: process.env.NETLIFY_URL,
},
plugins: [
'gatsby-plugin-uninline-styles',
Copy link
Member

Choose a reason for hiding this comment

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

This plugin does the same thing you did in the beginning? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the filename already contains the hash. I see.

@pmelab pmelab merged commit 2f2cb12 into release Jan 24, 2024
4 checks passed
@pmelab pmelab deleted the SLB-201-reduce-copying branch January 24, 2024 12:18
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.

2 participants