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

Add CSS generation on-the-fly #2050

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Add CSS generation on-the-fly #2050

wants to merge 20 commits into from

Conversation

andrewleith
Copy link
Member

@andrewleith andrewleith commented Jan 17, 2025

Summary | Résumé

This PR updates our frontend build pipeline so that we can take better control of it. In particular, these changes enable us to implement better developer experience by allows us to generate our tailwind + css on the fly as we are developing in a few seconds, vs. waiting the 25s+ it was taking before to run npm run tailwind

Improvements

  • New npm script to watch for changes and rebuild CSS (npm run watch) 🎉
  • Updated makefile command to do this automatically when you use make run-dev 🎉
  • Smaller bundled CSS size 🎉
  • Increased productivity for frontend tasks 🎉 🎉

Build pipeline before

    graph LR
    
    subgraph js[Webpack / JS Pipeline]
        direction TB
        J0[style-loader: extract CSS from JS]
        J0 --> J1[babel-loader: Convert ES6+ to ES5]
        J1 --> J2[minification: Compress JS]
    end

    subgraph css[Webpack / CSS Pipeline]
        direction TB
        P0[css-loader: handles css imports]
        P0-->P1[style-loader: takes CSS from css-loader and injects into page]
        P1-->P2[postcss-loader: calls postcss]
        P2 --> P3[MiniCssExtractPlugin: Extract to .css file]
    end

    subgraph postcss[PostCSS]
        direction TB
        PC2[postcss-import: Combine @import files]
        PC2 --> PC3[tailwindcss: Generate utility classes]
        PC3 --> PCN[cssnano: minify css]
        PCN --> PC4[autoprefixer: Add vendor prefixes]
        PC4 --> PC5[purgecss: Remove unused CSS]
    end
    

    subgraph Gulp[Gulp]
        CB[Compile] --> CSS
        CB --> JavaScript
        CB --> Assets
        
        subgraph Assets
            direction TB
            E1[Copy Images] --> E2
            E2[Copy Fonts] --> E3
            E3[Copy Gov Assets]
        end
        
        subgraph JavaScript
            direction TB
            D1[Concatenate] --> D2
            D2[Minify] --> D3
            D3[Source Maps]
        end
        
        subgraph CSS
            direction TB
            C3[Minify]
        end
    end
    js --> css --> postcss --> Gulp
Loading

Build pipeline after

graph LR
    subgraph tw[Tailwind]
        direction TB
        TW1[Compile CSS @imports]
        TW1 --> TW2[Remove unused CSS]
        TW2 --> TW3[Minify CSS]
    end

    subgraph js[Webpack / JS Pipeline]
        direction TB
        J0[style-loader: extract CSS from JS]
        J0 --> J1[babel-loader: Convert ES6+ to ES5]
        J1 --> J2[minification: Compress JS]
    end

    subgraph Gulp[Gulp]
        
        
        subgraph Assets
            direction TB
            E1[Copy Images] --> E2
            E2[Copy Fonts] --> E3
            E3[Copy Gov Assets]
        end
        
        subgraph JavaScript - all.min.js
            direction TB
            D1[Concatenate most internal js files] --> D2
            D2[Babel plugin] --> D3
            D3[Concatenate js from libs we depend on] --> D4
            D4[Minify] --> D5
            D5[Output to /static/javascripts/all.min.js]
        end
        
        subgraph JavaScript - indiviual components
            direction TB
            JS2[Babel plugin] --> JS4
            JS4[Minify using Uglify] --> JS5
            JS5[Output invidual files to /static/javascripts]
        end
        
        subgraph CSS
            direction TB
            C1[Minify using cleanCSS] --> C3
            C3[Output to /static/stylesheets/index.css]
        end
    end
    tw --> js --> Gulp
Loading

Quick launch of notify for debugging

Rebind F5 keyboard shortcut for admin only

  • Press cmd+shift+p and type `Open keyboard shortucts (JSON)
  • Add the following:
{
        "key": "f5",
        "command": "workbench.action.tasks.runTask",
        "args": "Dev Environment",
        "when": "config.useAdminDebug"
    }

Now when you press F5 in admin it will do the task - other repos will be unaffected!

⚠️ NOTE Ensure you select the "Python: Remote attach" entry from the Run & debug menu before pressing F5!

Copy link

@andrewleith andrewleith marked this pull request as ready for review January 20, 2025 19:26
@andrewleith andrewleith changed the title Task/better dx Add CSS hot-reloading Jan 21, 2025
@andrewleith andrewleith changed the title Add CSS hot-reloading Add CSS generation on-the-fly Jan 28, 2025
@smcmurtry smcmurtry self-requested a review January 28, 2025 19:42
Copy link
Contributor

@smcmurtry smcmurtry left a comment

Choose a reason for hiding this comment

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

I tested this locally and if I run poetry run flask run -p 6012 --host=localhost and npm run watch in separate terminals it all works as expected! The css is regenerated and reloading the browser gets the new styles.

I did have to run npm run tailwind first to generate the static assets, so I think we should change npm run build to npm run tailwind over here: https://github.com/cds-snc/notification-admin/blob/main/.devcontainer/scripts/installations.sh#L53

The debugger config also worked.

I am seeing an error at the end of the dev container set up script but that is not related to your PR.

@@ -53,7 +53,8 @@ coverage: venv ## Create coverage report

.PHONY: run-dev
run-dev:
poetry run flask run -p 6012 --host=localhost
npm run watch & \
Copy link
Contributor

Choose a reason for hiding this comment

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

This part isn't working for me - can we add this to a new make watch command?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, ill make those changes!

pyproject.toml Outdated Show resolved Hide resolved
@andrewleith
Copy link
Member Author

I did have to run npm run tailwind first to generate the static assets, so I think we should change npm run build to npm run tailwind over here: https://github.com/cds-snc/notification-admin/blob/main/.devcontainer/scripts/installations.sh#L53

Could you expand on this? I tried deleting my static/ folder, and then rebuilding the devcontainer. When I did, everything loaded normally.

I don't mind making this change, because its probably what should be in that script anyway (npm run build only runs gulp) but I'm just worried we're missing some other bug.

@smcmurtry
Copy link
Contributor

@andrewleith okay I tried again and I think you are right. When building a new dev container, I was seeing .devcontainer/scripts/installations.sh fail when running poetry run pre-commit install, which is unrelated to your PR. Once I commented that line out, I am seeing the app/static folder being created correctly and the app loading in the web browser. All good!

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.

3 participants