Support sass-loader v17 - #1141
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR expands the optional Changessass-loader v17 support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR widens the optional
Confidence Score: 5/5Safe to merge — changes are limited to widening an optional peer range; existing Node 20 users are unaffected and no loader logic was modified. The diff touches only peer dependency declarations and documentation. The Sass loader rule itself is unchanged, and the existing test suite already asserts that v17 maps to the correct loadPaths code path. The only cosmetic issue is the #XXXX placeholder in the changelog entry. CHANGELOG.md — the #XXXX PR placeholder needs to be updated to #1141 before the release is cut. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App installs sass-loader] --> B{Node.js version?}
B -->|Node 20.19.x| C["Resolves sass-loader ^16.x\n(highest matching v16)"]
B -->|Node 22.12+| D["Can opt into ^17.x\n(highest matching v17)"]
C --> E["Sass rule: loadPaths + api: modern\n(v16+ branch)"]
D --> E
E --> F[Builds successfully]
G["lib/install/package.json\n(installer default: ^16.x)"] -->|New app generated| C
style D fill:#d4edda,stroke:#28a745
style C fill:#fff3cd,stroke:#ffc107
Reviews (1): Last reviewed commit: "Fill in PR number for sass-loader v17 ch..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ### Added | ||
|
|
||
| - **Added support for `sass-loader` v17**. [PR #1141](https://github.com/shakacode/shakapacker/pull/1141) by [fukayatsu](https://github.com/fukayatsu). Widened the optional `sass-loader` peer range to `^13.0.0 || ^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0` in core `shakapacker`, `shakapacker-webpack`, and `shakapacker-rspack`. The Sass rule already selects `loadPaths` for v16+ and keeps `api: "modern"`, both of which remain valid in v17. Note that sass-loader v17 requires Node.js 22.11.0+ and drops `node-sass` and the legacy Sass JS API, so apps that opt into v17 must already be on Node 22.12+ (the upper branch of Shakapacker's `engines.node` range). |
There was a problem hiding this comment.
The changelog entry still uses #XXXX as the PR number placeholder rather than the actual PR number (#1141). Based on the project's changelog format guidelines, this should be [PR #1141](https://github.com/shakacode/shakapacker/pull/1141).
There was a problem hiding this comment.
The placeholder was already replaced with #1141 in 35d6019.
coderabbitai
Bot
added
documentation
p2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Thank you @fukayatsu! I just kicked off CI. |
|
@fukayatsu got some CI issues |
35d6019 to
80134b8
Compare
|
The |
|
@fukayatsu thanks! Fixing and I'll push a release asap. |
|
@fukayatsu rebase as #1142 merged. |
80134b8 to
8eef9e7
Compare
|
@justin808 Rebased onto upstream/main, thanks! |
|
@claude please review this PR. Focus on:
Please post your review as a PR comment, and use inline comments for specific code issues. |
Widen the optional sass-loader peer range to include ^17.0.0 in core shakapacker, shakapacker-webpack, and shakapacker-rspack, and update the peer-dependency and dependency-strategy docs to match. No rule changes are needed: the Sass rule already selects loadPaths for v16+ and keeps api: "modern", both of which remain valid in v17. v17 requires Node.js 22.11.0+ and drops node-sass and the legacy Sass JS API, so the addition is non-breaking — apps that opt into v17 already satisfy the upper branch of engines.node (>=22.12.0), while Node 20 users stay on v16 via the OR range. The pinned devDependency stays at 16.0.7 because CI runs on Node 20.19.x. The installer (lib/install/package.json) is intentionally NOT widened to ^17.0.0: its common range is the dependency actually installed into new apps (package managers install the highest match), so defaulting new installs to sass-loader 17 would force Node 22.12+ on apps that Shakapacker still supports on Node 20.19. v17 stays opt-in via the peer range. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8eef9e7 to
9950c91
Compare
|
Rebased onto main to resolve the CHANGELOG.md conflict. |
|
@claude please review the latest commit on this PR. Focus on:
Post your review as a PR comment, and use inline comments for specific code issues. |
|
Claude finished @justin808's task in 1m 50s —— View job PR Review: Support sass-loader v17
SummaryThis is a clean, minimal PR that correctly widens the Code Quality & Correctness ✅Sass rule logic handles v17 correctly ( const optionKey = packageMajorVersion("sass-loader") >= 16 ? "loadPaths" : "includePaths"The All three package manifests updated consistently — Installer intentionally left at v16 —
Test CoverageExisting test("sass-loader v17+ uses loadPaths", () => {
expect(getOptionKey(17)).toBe("loadPaths")
})One minor observation (pre-existing, not introduced by this PR): Security ✅No security implications. This is a peer dependency range widening with no new code paths. The change is purely additive and opt-in. Performance ✅No runtime performance impact. Peer dependency ranges have no effect at build or runtime. DocumentationChangelog, VerdictLGTM. The implementation is correct, the reasoning is well-documented, and the conservative installer default correctly protects Node 20 users. The only optional improvement would be a |
|
Thanks @fukayatsu |
|
@justin808 Thank you! |
…nalyze-webpack-output * origin/main: (164 commits) Make Rspack the default bundler for new installs (#1150) Add "Why Shakapacker with Rspack" comparison guide (#1155) Support sass-loader v17 (#1141) [codex] Fix Claude workflow permissions (#1153) Fix README CI badges and add prominent docs link (#1152) Trim README into a docs-site index; relocate content to docs/ and fix deep links (#1148) Tighten helper binstub parity and PATH lookup (#1128) Optimize CI: minimal version matrix on PRs, full matrix on main (#1151) Fix instance-scoped compiler strategies (#1147) Fix Rspack devServer in static watch mode (#1142) Add homepage links for supplemental npm packages (#1145) Release 10.1.0 Update CHANGELOG.md for v10.1.0 (#1139) Expand prerelease-to-stable changelog coalesce guidance in /update-changelog (#1138) Release 10.1.0-rc.2 [codex] Update changelog for v10.1.0-rc.2 (#1136) Teach /update-changelog to drop RC-only regression fixes (#1125) Restructure supplemental package dependencies (#1131) (#1133) [codex] Add Shakapacker brand assets (#1135) [codex] Fix shakapacker config helper binstubs (#1132) ...
* origin/main: (26 commits) Adopt agent-workflow binstubs (.agents/bin/ + AGENTS.md pointer) (#1176) Fix Ruby 3.1 CI: exclude broken i18n 1.15.0/1.15.1 (Fiber[]) (#1171) Add AI analysis prompt generator to config exporter (#695) Make Rspack the default bundler for new installs (#1150) Add "Why Shakapacker with Rspack" comparison guide (#1155) Support sass-loader v17 (#1141) [codex] Fix Claude workflow permissions (#1153) Fix README CI badges and add prominent docs link (#1152) Trim README into a docs-site index; relocate content to docs/ and fix deep links (#1148) Tighten helper binstub parity and PATH lookup (#1128) Optimize CI: minimal version matrix on PRs, full matrix on main (#1151) Fix instance-scoped compiler strategies (#1147) Fix Rspack devServer in static watch mode (#1142) Add homepage links for supplemental npm packages (#1145) Release 10.1.0 Update CHANGELOG.md for v10.1.0 (#1139) Expand prerelease-to-stable changelog coalesce guidance in /update-changelog (#1138) Release 10.1.0-rc.2 [codex] Update changelog for v10.1.0-rc.2 (#1136) Teach /update-changelog to drop RC-only regression fixes (#1125) ... # Conflicts: # CHANGELOG.md # lib/install/bin/diff-bundler-config # lib/install/bin/shakapacker-config # lib/tasks/shakapacker/export_bundler_config.rake # package/configExporter/cli.ts # spec/dummy/bin/shakapacker-config
## Summary Stamps the **`v10.2.0`** release section in `CHANGELOG.md` and adds the user-visible entries that were still missing for PRs merged since `v10.1.0`. Header format matches the repo convention (`## [v10.2.0] - July 3, 2026`) and is parseable by `rakelib/release.rake`'s `extract_changelog_section` (`## [v<npm-version>]`), so `bundle exec rake release` / `sync_github_release` will pick up the notes automatically. ### Changelog changes - **Version header**: inserted `## [v10.2.0] - July 3, 2026` immediately after `## [Unreleased]`; all accumulated entries now live under it, and `## [Unreleased]` is empty. - **Compare links**: `[unreleased]` now compares `v10.2.0...main`; added `[v10.2.0]: …/compare/v10.1.0...v10.2.0`. ### New entries added (were missing) | PR | Section | Note | | --- | --- | --- | | [#1187](#1187) | Added | Babel 8 peer dependency support + preset option compatibility | | [#1184](#1184) | Added | Folded into the #695 AI-prompt entry (gates the React on Rails section on app detection) | | [#1142](#1142) | Fixed | Rspack dev-server config no longer loads in static watch mode (fixes #1137) | ### Already documented (carried into v10.2.0) `#1180`, `#695`, `#1141`, `#1150`, `#1179`, `#1192`, `#1127`, `#1178`, `#1161`, `#1147`. ### Reviewed and intentionally excluded (not user-visible) Docs: `#1145`, `#1148`, `#1152`, `#1155`, `#1183`, `#1188`, `#1189`, `#1193`. CI: `#1151`, `#1168`, `#1171`. Tests / fixtures: `#1128`, `#1154`, `#1167`, `#1186`. Workflow / agent tooling: `#1153`, `#1176`, `#1182`. (`#1107` is already documented under `## [v10.1.0]`.) ## Next step After merge, run the repo's release task (no args) — it reads `v10.2.0` from the changelog and creates the GitHub release from this section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Summary
Widens the optional
sass-loaderpeer range to include^17.0.0so apps on Node 22.12+ can opt into sass-loader v17. No loader-rule changes are required.Closes #1140.
Changes
^17.0.0to the optionalsass-loaderpeer range in coreshakapacker,shakapacker-webpack, andshakapacker-rspack.docs/peer-dependencies.mdanddocs/dependency-strategy.mdto match.Why no rule changes: the Sass rule already selects
loadPathsfor sass-loader v16+ and keepsapi: "modern". Both remain valid in v17 — v17 only removed the legacy Sass JS API andnode-sass, neither of which Shakapacker uses.Why the installer is intentionally NOT widened:
lib/install/package.json'scommonrange is the dependency actually installed into newly generated apps (package managers install the highest match). Adding^17.0.0there would force every new app ontosass-loader@17, which requires Node ≥ 22.11.0 — combined with Shakapacker'sengines.node(^20.19.0 || >=22.12.0) that effectively means Node 22.12+. Since Shakapacker still supports Node 20.19, defaulting new installs to v17 would break the install/build flow for Node 20 users. An OR range can't express "allow v17 but install v16", so v17 stays opt-in via the peer range and the installer default stays conservative.Pull Request checklist
test/package/rules/sass-version-parsing.test.jsalready asserts v17 →loadPaths; generator specs exercise the install across npm/yarn/pnpm/bunOther Information
sass-loader@17requires Node.js 22.11.0+, dropsnode-sass, and removes the legacy Sass JS API. Apps using Shakapacker + v17 need Node 22.12+ (the upper branch of Shakapacker's supported range). Non-breaking: Node 20 users stay on v16 via the OR range.sass-loaderdevDependency stays at16.0.7because CI runs on Node 20.19.x andsass-loader@17requires Node 22.11+. Bumping it would require raising the whole CI Node matrix, out of scope for a compatibility-range change.yarn test(full JS suite) ✓,yarn lint+ Prettier ✓,bundle exec rspec spec/generator_specs/generator_spec.rb→ 71 examples, 0 failures.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation