Skip to content

Rework CodeMirror theme build system#4609

Open
G-Ambatte wants to merge 9 commits intonaturalcrit:masterfrom
G-Ambatte:reworkCMThemeBuildSystem
Open

Rework CodeMirror theme build system#4609
G-Ambatte wants to merge 9 commits intonaturalcrit:masterfrom
G-Ambatte:reworkCMThemeBuildSystem

Conversation

@G-Ambatte
Copy link
Copy Markdown
Collaborator

Description

This PR changes the buildHomebrew.js script, extracting the functions that update the CodeMirror themes files. This allows these build functions to be run independently of the main build script, if desired.

QA Instructions, Screenshots, Recordings

  • The buildHomebrew.js build script is modified, but Homebrewery will build and run as normal.
  • A new npm run command, npm run buildcm, is added, which will only update editorThemes.json if required.
  1. editorThemes.json is changed and does not match current files:
    a. Modify the array stored in themes/codeMirror/editorThemes.json.
    b. Run npm run buildcm.
    c. Receive console message Updating CM themes file: ./themes/codeMirror/editorThemes.json.
    d. Observe that the editorThemes.json file is rebuilt with the array matching the provided CSS files.
  2. Current files have changed and editorThemes.json is out of date:
    a. Add a new file with the file path and name themes/codeMirror/customThemes/000-test.css
    b. Run npm run buildcm.
    c. Receive console message Updating CM themes file: ./themes/codeMirror/editorThemes.json.
    d. Observe that the editorThemes.json file is rebuilt with the array matching the provided CSS files - default is always the first item in the array, so 000-test should be the second.
  3. editorThemes.json and all CSS files are unchanged.
    a. Make no changes to the editorThemes.json file or any of the files in customThemes. Note the created/modified timestamps of editorThemes.json.
    b. Run npm run buildcm.
    c. Receive the console message No updates required to CM themes file: ./themes/codeMirror/editorThemes.json
    d. Observe that the file, including created/modified timestamps, are all unchanged.
  4. editorThemes.json does not exist.
    a. Delete or rename themes/codeMirror/editorThemes.json.
    b. Run npm run buildcm.
    c. Observe that the file is created and contains an array of all CodeMirror theme CSS files.
  5. Homebrewery will build and run.
    a. Run npm run build as normal.
    b. Observe that the script completes successfully.
    c. Run npm start.
    d. Observe that the Homebrewery server starts as normal.
    e. Navigate to the Editor page, and confirm that you can select CodeMirror themes from the dropdown, and the relevant styling is applied.

Reviewer Checklist

  • Change editorThemes.json; confirm file is rebuilt when buildcm runs
  • Delete editorThemes.json; confirm file is rebuilt when buildcm runs
  • Change theme files; confirm file is rebuilt when buildcm runs
  • Change nothing; confirm file is not changed when buildcm runs
  • Homebrewery builds and runs as normal, including CodeMirror themes

@G-Ambatte G-Ambatte self-assigned this Jan 25, 2026
@G-Ambatte G-Ambatte added P3 - low priority Obscure tweak or fix for a single user 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed javascript Pull requests that update javascript code labels Jan 25, 2026
@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

The changes to admin.api.spec.js and brewRenderer.jsx are unintended. I'll push a commit soon that reverts those changes.

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Feb 8, 2026

Just for context, is the main goal here to avoid rebuilding the editorThemes.json when its' unchanged? Or, what is the use case for updating the themes files separately from the main build process? I don't fully understand the goal for this one.

Also, I'm curious what the specific improvement is here that I should be testing for, over the original brute-force method of just copying all the files, and then rebuild the json from the files found in the destination folder?

I say this because I have a suspicion that the steps of reading the theme files, sorting them, reading in the previous .json, and then comparing, takes more time than just immediately rebuilding it. What do you think?

@calculuschild calculuschild added 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs re-evaluation or clarification and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Feb 8, 2026
@5e-Cleric
Copy link
Copy Markdown
Member

The build file that made this possible is now set as a plugin that runs after clearing the build folder but before the build process starts. Not sure how necessary is this now. I still think its weird we point towards the build folder for a file in our code instead of having a copy in the themes folder that gets rebuilt over time, but as long as it works...

Copy link
Copy Markdown
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

Can you help me out understanding the goal of this PR?

@5e-Cleric
Copy link
Copy Markdown
Member

Can you help me out understanding the goal of this PR?

When i was building the vite PR, i realized we take the codemirror themes from a file in the build folder. This seemed very weird to me at the time and i made a comment about it. Gambatte agreed and built this to separate build from making the codemirror theme json.

At this moment on stage its already separated into a plugin, but it is run with build. I am no longer against the process, although i am still unsure as to why we would make the file into the build folder.

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Mar 18, 2026

i am still unsure as to why we would make the file into the build folder.

"Build" is the deployment contents, and what Heroku considers the "root" path of the site. So all content being served to users should ideally live inside there (hence why we also copy all the brew themes over there as part of building).

The intent is to not be serving resources directly from source code folders.

@5e-Cleric
Copy link
Copy Markdown
Member

Incidentally, if #4689 is merged, this PR will turn completely obsolete. Sorry not sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update javascript code P3 - low priority Obscure tweak or fix for a single user 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs re-evaluation or clarification

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants