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

[CIVIC-1946] Added minimal dependency build system. #1313

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

alan-cole
Copy link
Collaborator

@alan-cole alan-cole commented Oct 16, 2024

https://salsadigital.atlassian.net/browse/CIVIC-1946

Checklist before requesting a review

  • I have formatted the subject to include ticket number as Issue #123456 by drupal_org_username: Issue title
  • I have added a link to the issue tracker
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. Added a minimal dependency build system to eventually replace the gulp / webpack system civictheme currently uses.
  2. Can be run using npm run build:new and watch can be enabled by running npm run build:watch:new.
  3. Currently supports base and subtheme builds.
  4. This will rely on some of the code changes in UI Kit - Updated button mixins to live in base for improved re-usability. uikit#408 and [#392] Inline CSS dependencies. uikit#402

Notes:

Civictheme build - version 0.0.1 (alpha).

This is a simplified build system for civictheme, designed to:

- Speed up build time
- Improve auditability by reducing dependencies / external code

Build by calling:

  npm run build:new

Build and watch by calling:

  npm run build:watch:new

Notes:

- Requires Node version 22 (for the built in glob feature).
- No Windows support (unless through WSL).
- This relies on command line rsync.
- This does not support uglify or minifying JS / CSS code.
- Watch does not trigger on a directory change, only on a (scss, twig, js) file change.

Screenshots

Screenshot 2024-10-16 at 5 01 33 pm
Screenshot 2024-10-16 at 5 02 24 pm

@alan-cole alan-cole added State: DO NOT MERGE Do not merge this pull request State: Needs review Pull requests needs a review from assigned developers and removed State: DO NOT MERGE Do not merge this pull request labels Oct 16, 2024
@AlexSkrypnyk
Copy link
Contributor

The 1 second build looks amazing!

Could you please explain how will this work with Storybook?
The reason we used Webpack initially was to have the same build tooling for building assets as building Storybook: we only needed to adjust a couple of entry points for Storybook's webpack config and the rest was re-used as-is.

const DIR_ASSETS_OUT = fullPath('./dist/assets/')

const COMPONENT_DIR = config.base ? DIR_COMPONENTS_IN : DIR_COMPONENTS_OUT
const STYLE_NAME = config.base ? 'civictheme' : 'styles'
Copy link
Contributor

Choose a reason for hiding this comment

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

nice


const PATH = __dirname

const THEME_NAME = PATH.split('/').reverse()[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

i think adding comments for each variable will really help everyone to understand the purpose


// --------------------------------------------------------------------------- ASSETS
if (config.assets) {
runCommand(`rsync -a --delete --exclude js --exclude sass ${DIR_ASSETS_IN}/ ${DIR_ASSETS_OUT}/`)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be windows-compatible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently no, though we could look into the use of robocopy with a -win flag as an option for Windows developers.

@alan-cole
Copy link
Collaborator Author

alan-cole commented Nov 11, 2024

The 1 second build looks amazing!

Could you please explain how will this work with Storybook? The reason we used Webpack initially was to have the same build tooling for building assets as building Storybook: we only needed to adjust a couple of entry points for Storybook's webpack config and the rest was re-used as-is.

Still looking into how this will work with Storybook (this is one of the reasons why the webpack version still exists).

But one option I'm looking at is importing the dest css and js files into storybook, removing the need to apply the build in storybook.

A developer could run npm run build:watch:new to watch for component changes (which would update the dest files when a component changes), and also run npm run storybook at the same time, which would pick up on dest changes and reload the story.

If this could be simplified so a single npm command can run both services, that would be ideal.

If there are better alternatives available in storybook to allow custom build system to be configured, we should explore that as well.

@alan-cole alan-cole force-pushed the feature/CIVIC-1946-new-build branch from 4fcdb68 to dbd6f3c Compare November 26, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Needs review Pull requests needs a review from assigned developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants