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

feature/issue 1257 CSS optimization workflow parity #1258

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Jul 17, 2024

Related Issue

resolves #1257

Summary of Changes

  1. Support inlining @import statements in development (too)
  2. This also does all production optimizing too during development
  3. Refresh PostCSS plugin README
  4. Update styles and assets docs

TODO

  1. Update test cases based on outcome of this being default (albeit) documented behavior

  2. Certain test cases will depend on enhancement/issue 923 real production import attributes #1259 being merged

  3. figure out how to get outputDir from configuration - not needed, its not configurable anyway

  4. looks like development optimization strips content in one test case? - looks to be an issue with csstree

         + expected - actual
    
     -constraw=`*{background-image:url('/assets/background.jpg');font-family:'Arial';}.blockquote-footer::before{content:'— '}.fa-chevron-right:before{content:''}`;exportdefaultraw;
     +constraw=`*{background-image:url('/assets/background.jpg');font-family:'Arial';}.blockquote-footer::before{content:"\\2014\\00A0";}.fa-chevron-right:before{content:"\\f054";}`;exportdefaultraw;
    
  5. Update documentation to call this out

  6. final upstream testing - done in enhancement/issue 923 real production import attributes #1259

  7. @custom-media parsing error - coming from @evergreen-wc looks like we needed a PostCSS plugin for this? 😅 - https://github.com/csstools/postcss-custom-media

         Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
     -------------------------------------------^
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
     Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
         2 |@custom-media --screen-sm (min-width: 576px);
     -------------------------------------------^
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
     Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
     -------------------------------------------^
         4 |@custom-media --screen-lg (min-width: 992px);
         5 |@custom-media --screen-xl (min-width: 1200px);
     Parse error: ")" is expected
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
     -------------------------------------------^
         5 |@custom-media --screen-xl (min-width: 1200px);
         6 |
     Parse error: ")" is expected
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
         5 |@custom-media --screen-xl (min-width: 1200px);
     -------------------------------------------^
         6 |
         7 |:host .container {
     Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
     -------------------------------------------^
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
     Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
         2 |@custom-media --screen-sm (min-width: 576px);
     -------------------------------------------^
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
     Parse error: ")" is expected
         1 |@custom-media --screen-xs (max-width: 576px);
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
     -------------------------------------------^
         4 |@custom-media --screen-lg (min-width: 992px);
         5 |@custom-media --screen-xl (min-width: 1200px);
     Parse error: ")" is expected
         2 |@custom-media --screen-sm (min-width: 576px);
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
     -------------------------------------------^
         5 |@custom-media --screen-xl (min-width: 1200px);
         6 |
     Parse error: ")" is expected
         3 |@custom-media --screen-md (min-width: 768px);
         4 |@custom-media --screen-lg (min-width: 992px);
         5 |@custom-media --screen-xl (min-width: 1200px);
     -------------------------------------------^
         6 |
         7 |:host .container {
       
  8. reduced motion parsing - seems to only be an issue when using puppeteer, and so maybe this is needed as part of another PR (also I think our puppeteer plugin is pretty outdated)? - https://pptr.dev/api/puppeteer.page.emulatemediafeatures

         Parse error: Number, dimension, ratio or identifier is expected
          57 |}
          58 |
          59 |@media (prefers-reduced-motion:) {
       --------------------------------------^
          60 |app-banner .banner.app-banner span.off.app-banner {
          61 |  animation:0
       
  9. font bundling - should have been "gating" the plugin against file:// protocol only URLs

         TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file
           at open (node:internal/fs/promises:581:10)
           at Object.readFile (node:internal/fs/promises:1027:20)
           at StandardFontResource.serve (file:///Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/plugins/resource/plugin-standard-font.js:25:27)
           at file:///Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/serve.js:69:40
           at async file:///Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/serve.js:55:5 {
         code: 'ERR_INVALID_URL_SCHEME'
       }
       
  10. we should probably be optimizing one-offs... (but how do we run async plugins inside a sync AST walker 🤔 )

Questions

  1. does this need outputDir available in loader.js? - yes!
  2. Should we make it optional since it is not standard behavior? Is this juice worth the squeeze? - not for now, since it brings a lot of immediate value
    • we were doing it for production...
  3. Should we avoid any excess operations? (though really, once you turn it into an AST, the heavy lifting is really already there), like - not for now, but we can easily make it optional if it becomes an issue down the road, plus parity between dev and build environments is pretty important
    • minification - dev tools can format I think?
    • hashing - moderately expensive, but not really needed either way (use fs etc)
  4. Somewhat related, but staticServer is using shouldServe lifecycle, which was giving some unexpected results when running yarn serve and seemed to be bundling even when we weren't using (this kind of relates a a need / idea around standalone server, which I should also track) - put into my personal backlog
    • yeah npx http-server ./public breaks
    • npx http-server loads all the content just fine, nowstandard resource plugins needed. So do we even need resource plugins for staticStatic
  5. add a tracking item for content with octal values? - Lightning CSS #1238 (comment)

@thescientist13 thescientist13 added question Further information is requested documentation Greenwood specific docs CLI feature New feature or request labels Jul 17, 2024
@thescientist13 thescientist13 changed the title feature/issue 1257 css optimization workflow parity feature/issue 1257 CSS optimization workflow parity Jul 17, 2024
@thescientist13 thescientist13 self-assigned this Jul 20, 2024
@jstockdi jstockdi requested a review from DevLab2425 July 24, 2024 23:24
@thescientist13 thescientist13 force-pushed the feature/issue-1257-css-optimization-workflow-parity branch from 94ad07c to e237f50 Compare July 31, 2024 02:45
@thescientist13 thescientist13 marked this pull request as ready for review July 31, 2024 02:45
@thescientist13 thescientist13 removed the question Further information is requested label Aug 2, 2024
@thescientist13 thescientist13 merged commit 12c9793 into release/0.30.0 Aug 2, 2024
8 checks passed
@thescientist13 thescientist13 deleted the feature/issue-1257-css-optimization-workflow-parity branch August 2, 2024 17:31
thescientist13 added a commit that referenced this pull request Nov 2, 2024
* initial implementation

* WIP updating test cases

* refresh PostCSS plugin README docs

* document optimization in development behavior

* restore test case with exception

* tests passing

* gate font plugin from only serving files

* optimize supported one-off externalized assets

* restore disabled test cases

* fixup comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI documentation Greenwood specific docs feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS optimization workflow parity during development (inlining @import statements)
1 participant