Skip to content

Conversation

@elektronaut
Copy link

Summary

Server rendering currently raises an undefined method '[]' for an instance of Propshaft::Assembly error when using the Propshaft assets pipeline, since it assumes Sprockets is present.

This PR adds a new PropshaftContainer adapter. According to the Propshaft documentation, this API works the same for both precompiled and dynamic assets.

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

@justin808
Copy link
Collaborator

Claude Code Review

Summary

This PR successfully adds support for Propshaft asset pipeline in server-side rendering by implementing a new PropshaftContainer adapter. The implementation follows existing patterns, includes proper test coverage, and integrates well with the CI pipeline. Overall, this is a well-crafted addition that expands asset pipeline compatibility.

Key Findings

Strengths

  • Clean implementation: PropshaftContainer follows the same pattern as existing containers (SprocketsContainer, EnvironmentContainer)
  • Proper compatibility check: Uses compatible? class method to detect Propshaft presence (lib/react/server_rendering/propshaft_container.rb:12-14)
  • Good test coverage: Includes unit test with PropshaftHelpers guard (test/react/server_rendering/propshaft_container_test.rb:1-14)
  • CI integration: Adds propshaft to test matrix (.github/workflows/ruby.yml:76)
  • Complete PR checklist: Tests, docs, and CHANGELOG all properly updated
  • Encoding safety: Properly forces UTF-8 encoding on asset content (lib/react/server_rendering/propshaft_container.rb:18)

Suggestions for Enhancement

  1. Integration order documentation (lib/react/server_rendering/bundle_renderer.rb:107)

    • Consider adding a comment explaining the container fallback order priority
    • Current order: SeparateServerBundleContainer → PropshaftContainer → EnvironmentContainer → ManifestContainer
  2. Test coverage depth (test/react/server_rendering/propshaft_container_test.rb:10)

    • Current test only verifies content is not empty
    • Consider testing that content is valid JavaScript:
    assert_match(/function|var|const/, container.find_asset("server_rendering.js"))
  3. Error handling clarity (lib/react/server_rendering/propshaft_container.rb:17-18)

    • Consider adding explicit error handling for missing assets:
    def find_asset(path)
      self.class.assembly.load_path.find(path).content.force_encoding("utf-8")
    rescue => e
      raise "Asset not found: #{path}. Error: #{e.message}"
    end
  4. Documentation enhancement (README.md:13)

    • Consider specifying version requirement: "Supports Propshaft v1.0+"

Minor Notes

  • CI comment "These don't have shakapacker:" (.github/workflows/ruby.yml:75) is accurate but now applies to both propshaft and sprockets
  • Integration test for end-to-end server rendering with Propshaft would be valuable (though unit test is sufficient)

Code Quality

  • Maintainability: Excellent - follows established patterns
  • Readability: Clear and concise implementation
  • Architecture: Properly integrates into existing container selection logic

Testing

  • Unit test properly guards execution with PropshaftHelpers.when_propshaft_available
  • CI matrix ensures propshaft configuration is continuously tested
  • Appraisal configuration correctly specifies propshaft dependency

Security

No security concerns identified. Uses Propshaft's standard public API for asset loading.

Performance

Minimal performance impact. The compatible? check is efficient, and asset loading uses Propshaft's optimized mechanisms.

Documentation

  • CHANGELOG properly updated with PR reference
  • README updated to mention Propshaft support
  • Gemfile and Appraisals correctly configured

Breaking Changes

None - this is purely additive functionality.


Overall Assessment: This is a solid PR that cleanly adds Propshaft support. The implementation is well-tested, follows existing patterns, and integrates properly with the build system. The suggestions above are optional enhancements rather than blockers.

Recommendation: ✅ Approve


Automated review by Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants