Default to modern Sass API for sass-loader

[justin808]

Summary

• Default sass-loader to use api: 'modern' instead of the deprecated legacy API
• Improves compatibility with sass plugins like sass-resources-loader

Fixes #877

Test plan

• Existing sass-loader tests pass
• Added test verifying api: 'modern' is set

🤖 Generated with Claude Code

Summary by CodeRabbit

• Breaking Changes

  • sass-loader now defaults to the modern Sass API for improved plugin compatibility

• Fixed

  • NODE_ENV conflicts when RAILS_ENV=test are now properly resolved

• Documentation

  • Added instructions for reverting to the legacy Sass API if needed

[coderabbitai]

Walkthrough

This PR defaults sass-loader to the modern Sass API by setting api: "modern" in the configuration. It also fixes an issue where NODE_ENV=test caused DefinePlugin warnings by setting NODE_ENV to development when RAILS_ENV=test. Changes include configuration update, changelog documentation, and test coverage for the new behavior.

Changes

Cohort / File(s)	Summary
Configuration & Documentation CHANGELOG.md	Added breaking change notice for modern Sass API default and fix for NODE_ENV=test DefinePlugin warnings with configuration guidance.
Sass Loader Rule package/rules/sass.ts	Added api: "modern" option to sass-loader configuration to enable modern Sass API by default.
Test Coverage test/package/rules/sass1.test.js	Added test case validating that sass-loader uses the modern API for improved plugin compatibility.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Add logic to sass rule conditional on sass-loader version #508: Modifies Sass rule's loader configuration for sass-loader API/version differences by adding version-conditional logic for includePaths/loadPaths handling alongside this PR's modern API default setting.

Suggested reviewers

• tomdracz

Poem

🐰 From legacy chains we now break free,
Modern Sass API sets the spree!
Plugins rejoice, the warnings flee,
Shakapacker hops with compatibility! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR partially implements the requirements from issue #877: sets sass-loader default to api: 'modern' [#877] and updates CHANGELOG [#877], but omits the configurable shakapacker.yml option and detailed documentation requirements [#877].	Implement the remaining requirements: add a sass_api configuration option in shakapacker.yml and provide comprehensive troubleshooting documentation as specified in issue #877.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: defaulting sass-loader to the modern Sass API, which is the primary objective of this pull request.
Out of Scope Changes check	✅ Passed	All changes are directly related to the issue #877 objectives: CHANGELOG documents the breaking change, sass.ts sets the modern API default, and tests verify the configuration. No unrelated modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jg/fix-877

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Overview

Greptile Summary

This PR defaults sass-loader to use the modern Sass API instead of the deprecated legacy API, improving compatibility with sass plugins like sass-resources-loader.

Key changes:

• Added api: "modern" to sass-loader configuration in package/rules/sass.ts
• Added test coverage verifying the modern API setting in test/package/rules/sass1.test.js

Note: This PR is missing a CHANGELOG entry. Per project guidelines in CLAUDE.md, user-visible changes (like this compatibility improvement) should be documented in CHANGELOG.md under the Unreleased section.

Confidence Score: 4/5

• This PR is safe to merge with one minor procedural gap
• The code changes are minimal, well-tested, and non-breaking. The modern Sass API is a straightforward configuration change that improves plugin compatibility. Files properly end with newlines per project standards. Score is 4 (not 5) only because the CHANGELOG entry is missing per project guidelines.
• No files require special attention - all changes are straightforward and properly implemented

Important Files Changed

Filename	Overview
package/rules/sass.ts	Added api: "modern" to sass-loader options to use modern Sass API instead of deprecated legacy API
test/package/rules/sass1.test.js	Added test verifying api: "modern" is set in sass-loader options

Sequence Diagram

sequenceDiagram
    participant User as Developer
    participant Webpack as Webpack/Rspack
    participant SassRule as sass.ts
    participant SassLoader as sass-loader
    participant SassCompiler as Sass Compiler
    
    User->>Webpack: Build process initiated
    Webpack->>SassRule: Request Sass rule configuration
    SassRule->>SassRule: Check sass-loader version
    alt sass-loader v16 or later
        SassRule->>SassRule: Use loadPaths option key
    else sass-loader v15 or earlier
        SassRule->>SassRule: Use includePaths option key
    end
    SassRule->>SassLoader: Configure with modern API enabled
    Note over SassLoader: Modern Sass API provides<br/>better plugin compatibility
    SassLoader->>SassCompiler: Compile Sass files using modern API
    SassCompiler-->>SassLoader: Compiled CSS
    SassLoader-->>Webpack: Processed CSS output
    Webpack-->>User: Built assets with compiled styles

Loading

[claude]

PR Review: Default to Modern Sass API

✅ Overall Assessment

This is a well-implemented PR that makes a sensible default change. The code quality is excellent and follows project conventions.

---

Code Quality & Best Practices

Strengths:

• ✅ Clean, minimal change - only adds one line to the core code
• ✅ Follows existing code patterns in package/rules/sass.ts:12
• ✅ File ends with proper trailing newline (per CLAUDE.md requirements)
• ✅ Test follows existing test structure and naming conventions

Minor Observations:

• The new test in test/package/rules/sass1.test.js:23-25 correctly verifies the api: "modern" setting
• Consistent with other option configurations in the sass rule

---

Test Coverage

Strengths:

• ✅ New test added specifically for the modern API setting
• ✅ Test is properly structured and uses the same mocking pattern as existing tests
• ✅ Test is in the right file (sass1.test.js) alongside other sass-loader option tests

Potential Consideration:

• Since sass-loader is supported from v13-v16 (per package.json peerDependencies), and the modern API may have different behaviors across versions, consider if additional test coverage across sass-loader versions would be beneficial. However, the existing test structure (sass.test.js, sass1.test.js, sass16.test.js) already covers version-specific concerns, so this may be sufficient.

---

Potential Issues

None identified. The change is:

• Backward compatible via the documented override path in CHANGELOG.md
• Well-documented with clear migration instructions
• Solves a real user pain point (issue Enhancement: Default to modern Sass API for sass-loader #877)

---

Performance Considerations

✅ Positive Impact:

• The modern Sass API is the recommended approach by the sass-loader maintainers
• The legacy API is deprecated, so this change aligns with the ecosystem direction
• No performance regressions expected - the modern API is designed to be more efficient

---

Security Concerns

✅ None identified.

• This is a configuration change only
• No new dependencies or attack vectors introduced
• The modern API is the maintained, actively supported option

---

Documentation & Changelog

Strengths:

• ✅ Excellent CHANGELOG.md entry with:
  • Clear breaking change notice
  • Detailed explanation of the change and rationale
  • Comprehensive migration instructions with code example
  • Proper formatting following project conventions (uses # in PR links per CLAUDE.md)
• ✅ Links to both PR and user GitHub profiles
• ✅ Follows the project's changelog format exactly

Formatting:
The CHANGELOG entry correctly uses [PR #879](https://github.com/shakacode/shakapacker/pull/879) format as required by the project.

---

Rspack Compatibility

✅ Verified: This change works with both webpack and rspack since:

• The sass-loader is used by both bundlers via the getStyleRule utility
• The api option is a sass-loader configuration, not bundler-specific
• No bundler-specific logic needed

---

Recommendations

1. Before merging: Ensure the test suite passes (per CLAUDE.md, should run yarn test)
2. Consider: Adding a note in the documentation about sass-resources-loader compatibility, since that was the motivating use case from issue Enhancement: Default to modern Sass API for sass-loader #877
3. Future: When sass-loader eventually removes legacy API support entirely, this default will already be in place

---

Summary

This is a high-quality PR that:

• ✅ Solves a real user problem
• ✅ Makes a sensible default change
• ✅ Provides excellent migration documentation
• ✅ Has appropriate test coverage
• ✅ Follows all project conventions

Recommendation: Approve and merge (pending CI checks passing).

Great work @justin808! 🎉