Repository navigation
build: support use as an npm workspace package - #4442
brian-smith-tcril merged 5 commits into
Conversation
✅ Deploy Preview for paragon-openedx-v23 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #4442 +/- ##
================================================
- Coverage 94.25% 94.21% -0.05%
================================================
Files 242 242
Lines 4301 4305 +4
Branches 992 994 +2
================================================
+ Hits 4054 4056 +2
- Misses 243 245 +2
Partials 4 4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolve babel from PATH, add clean and watch:build, and report 0.0.0-dev as the checked-in version. Co-Authored-By: Claude <noreply@anthropic.com>
2aeb09c to
2a7da1f
Compare
Co-Authored-By: Claude <noreply@anthropic.com>
159889a to
7496a5e
Compare
Co-Authored-By: Claude <noreply@anthropic.com>
Split the build into build-js and build-scss, the latter keyed on its scss and theme inputs, and point watch:build at the incremental target. Compiling the stylesheets takes minutes; the JavaScript takes seconds. Co-Authored-By: Claude <noreply@anthropic.com>
7496a5e to
e392b0b
Compare
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Quick claude pass on this one (with my responses to claude's findings)
- The version change to
0.0.0-devis user-visible.lib/version.jsprints thepackage.jsonversion forparagon version, so a workspace checkout will report0.0.0-dev. The test inlib/__tests__/version.test.jsreads the same file, so it still passes. Probably acceptable given the release process stamps the real version, but worth a conscious yes.
This part makes sense to me. We've standardized on 0.0.0-dev for semantically released frontend stuff so this gets my 👍.
- The
THEME_SOURCESprerequisite list omitstokens/.nodemon.jsonwatchestokens, but the Make rule only rebuildsdist/theme-urls.jsonwhenstyles/orsrc/ scss/csschanges.build-scssdoes not read tokens directly, so this is consistent today. It only bites if someone expects a token edit to trigger a stylesheet rebuild in watch mode.
Functionally I'm cool with the prereqs as they are. The only nit I'd have here would be to maybe rename it from THEME_SOURCES to STYLE_SOURCES or STYLESHEET_SOURCES to better reflect what's actually coming from the find.
build: clean build-incrementalrelies on serial prerequisite order. Correct without-j, butmake -jcould run clean concurrently with the build steps. Low risk since nobody runs Paragon's build with-j, but abuild-incremental: | clean-style ordering or a recursive$(MAKE)would make it explicit.
I agree this is low risk, if addressing it makes the Makefile more confusing it doesn't feel worth it to me at the moment, but if it can be addressed simply why not?
- The new
~resolver inlib/build-scss.jsthrows on a bare~import.specifier.match(...)[0]dereferences a possibly-null match. No stylesheet in the repo does that, so it is a nit at most.
I asked "any suggestions for addressing the ~ resolver thing?" and got the following response. Considering the "no stylesheet in the repo does that" thing I'm happy to ignore this one.
Claude response to my follow-up question
The cleanest fix folds the tilde check and the package-name capture into one regex, so the guard that already exists does double duty and there is no separate null path to forget:
findFileUrl(url) { // '~<package>/...' where the package is an optional '@scope/' plus one segment. const match = url.match(/^~((?:@[^/]+\/)?[^/]+)/); if (!match) { return null; } const [, packageName] = match; const specifier = url.substring(1); const searchPaths = require.resolve.paths(packageName) || []; const dir = searchPaths.find((nodeModules) => fs.existsSync(path.join(nodeModules, packageName))); return dir ? pathToFileURL(path.join(dir, specifier)) : null; },Why this shape rather than just wrapping the
[0]in a null check:
- One guard instead of two. The original
startsWith('~')line becomes redundant, so the function gets shorter, not longer. A bare~, or~@scopewith no package segment, no longer matches and falls through toreturn null.- Returning null is the right failure mode. Sass's importer contract treats null as "not mine", so it moves on to load paths and then reports its standard "Can't find stylesheet to import" error with the file and line. That is a better message than a TypeError from inside the importer with a stack trace into
build-scss.js.- Keep the
require.resolve.pathsplusexistsSyncapproach. The tempting alternative ispath.dirname(require.resolve(${packageName}/package.json)), but packages with anexportsmap that does not listpackage.jsonwill throw on that, and Paragon's stylesheets already reach into bootstrap'sscss/directory in ways an exports map could block. Walking the candidatenode_modulesdirectories sidesteps that entirely.One thing worth confirming since it is the point of the PR:
require.resolve.pathsanchors on the location oflib/build-scss.js, so it walks up from Paragon's own directory. In a workspace that reaches the hoisted rootnode_modules, which is what you want. It would only misbehave if a consumer ran the script with a stylesheet whose tilde import named a package that is installed next to the stylesheet but not visible from Paragon, and nothing in the repo does that today.
Rename THEME_SOURCES to STYLESHEET_SOURCES, and make `build` invoke clean and build-incremental recursively so the ordering holds under -j. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed, thanks! |
|
🎉 This PR is included in version 23.23.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Paragon cannot currently be developed as a
packages/workspace of a site built onfrontend-base, which is howfrontend-baseand the MFEs themselves are developed. Three small things get in the way, all fixed here. Closes #4440.The
Makefilecalled./node_modules/.bin/babeldirectly, which does not exist under workspace hoisting. It now resolvesbabelfrom PATH, the way thetscline above it already does.A
cleantarget pluscleanandwatch:buildscripts givenpm run clean:packagesandnpm run dev:packagessomething to call, instead of silently no-opping for Paragon.The checked-in version becomes
0.0.0-dev, like every other Open edX frontend repo, leaving the real version to the release process.20.18.1onrelease-23.xis stale regardless, since semantic-release never commits the bump back.Compiling the stylesheets takes about three minutes here, against four seconds for everything else, so the build is now split into
build-jsandbuild-scss, with the latter keyed on its scss and theme inputs.buildstill cleans and does everything;watch:buildusesbuild-incremental, which skips the stylesheets unless they changed.LLM usage notice
Built with assistance from Claude.