Merge pull request #313 from rails/rm-fix-282

rafaelfranca · web-flow · commit 51c1a531327f · 2025-07-31T20:08:33.000-04:00
Fix `pin_all_from` incorrectly removing "js" substring from filenames
diff --git a/.github/prompts/fix-issue.prompt.md b/.github/prompts/fix-issue.prompt.md
@@ -0,0 +1,92 @@
+---
+mode: agent
+description: 'Fix an issue in the importmap-rails gem by following a systematic process.'
+---
+# GitHub Issue Fixer Prompt
+
+You are an expert Ruby developer specializing in fixing issues in the importmap-rails gem. Your task is to systematically analyze, test, and fix GitHub issues.
+
+Ask for the the issue number you are working on, then follow the steps below to resolve it.
+
+## Workflow
+
+1. **Fetch Issue Details**: Use `gh api` to retrieve the complete issue information
+2. **Analyze the Problem**: Understand the root cause from the issue description
+3. **Write Failing Tests**: Create comprehensive test cases that reproduce the issue
+4. **Implement the Fix**: Make minimal, targeted changes to fix the issue
+5. **Verify the Solution**: Ensure all tests pass and the fix works as expected
+
+## Commands to Use
+
+### Fetch Issue Information
+```bash
+# Get issue details
+gh api repos/rails/importmap-rails/issues/{issue_number}
+
+# Get issue comments (if any)
+gh api repos/rails/importmap-rails/issues/{issue_number}/comments
+```
+
+### Run Tests
+```bash
+# Run all tests
+bundle exec rake test
+
+# Run specific test file
+bundle exec rake test TEST=test/specific_test.rb
+
+# Run with verbose output
+bundle exec rake test TESTOPTS="-v"
+```
+
+## Project Context
+
+### Architecture
+- **Core Class**: `Importmap::Map` in `lib/importmap/map.rb`
+- **Key Methods**:
+  - `pin` - pins individual packages
+  - `pin_all_from` - pins all files from a directory
+
+### Testing Patterns
+- Use Minitest with `ActiveSupport::TestCase`
+- Test files are in `test/` directory
+- Tests use a setup method to create an `Importmap::Map` instance
+- Test naming convention: `test "description of what is being tested"`
+
+## Analysis Guidelines
+
+### Test Writing Guidelines
+1. **Reproduce the exact scenario** described in the issue
+2. **Test edge cases** and variations of the problem
+3. **Use descriptive test names** that explain the scenario
+4. **Include both positive and negative test cases**
+5. **Test the fix doesn't break existing functionality**
+6. **Don't add comments in the test code** - use clear method names instead
+
+## Fix Implementation Guidelines
+
+1. **Make minimal changes** - only fix what's broken
+2. **Preserve existing behavior** for non-broken cases
+3. **Don't add inline comments** anywhere in the codebase
+   - Use descriptive method and variable names instead
+   - Ensure code is self-explanatory
+4. **Follow Ruby and Rails conventions**
+
+## Verification Steps
+
+1. **Run existing tests** to ensure no regressions
+2. **Test the specific scenario** from the issue
+3. **Test edge cases** and similar scenarios
+4. **Verify in a Rails app** if possible (using test/dummy)
+5. **Check performance impact** for the change
+
+## Output Format
+
+When fixing an issue, provide:
+
+1. **Issue Analysis**: Brief explanation of the root cause
+2. **Test Cases**: The tests you wrote to reproduce the issue
+3. **Fix Implementation**: The actual code changes made
+4. **Verification**: Results of running tests and any additional validation
+
+Remember: Always write tests first, then implement the fix to make them pass. This ensures you truly understand and solve the problem.
diff --git a/.github/prompts/generate-commit-message.prompt.md b/.github/prompts/generate-commit-message.prompt.md
@@ -0,0 +1,122 @@
+---
+mode: agent
+description: 'Generate clear, descriptive commit messages for importmap-rails changes'
+---
+# Commit Message Generator
+
+You are an expert at writing clear, descriptive commit messages for the importmap-rails gem. Generate commit messages that follow the project's conventions and clearly communicate what was changed and why.
+
+## Commit Message Guidelines
+
+### Structure
+- **Subject line**: Clear, concise description of what was done (50-72 characters)
+- **Body**: Explain the problem, impact, and solution (wrap at 72 characters)
+- **Footer**: Reference issues with "Fixes #123" or "Closes #456"
+
+### Style Principles
+1. **Focus on WHAT was fixed, not HOW it was implemented**
+2. **Describe the user-facing problem and impact**
+3. **Use imperative mood** ("Fix", "Add", "Remove", not "Fixed", "Added", "Removed")
+4. **Be specific about the component** (pin_all_from, importmap generation, etc.)
+5. **Avoid implementation details** in the subject line
+
+### Common Patterns for importmap-rails
+
+#### Bug Fixes
+```
+Fix pin_all_from incorrectly processing filenames with [specific issue]
+
+[Describe the problem behavior and impact]
+[Explain what the fix accomplishes]
+
+Fixes #123
+```
+
+#### Feature Additions
+```
+Add support for [new capability]
+
+[Explain the use case and benefits]
+[Describe the new behavior]
+
+Closes #123
+```
+
+#### Security/Performance
+```
+Improve [security/performance aspect] for [component]
+
+[Explain the improvement and why it matters]
+
+Fixes #123
+```
+
+## Context for importmap-rails
+
+### Key Components
+- **Importmap::Map**: Core mapping logic in `lib/importmap/map.rb`
+- **pin/pin_all_from**: Methods for registering JavaScript modules
+- **Asset pipeline integration**: Sprockets/Propshaft compatibility
+- **CLI commands**: `./bin/importmap` for package management
+- **Helper methods**: Rails view helpers for generating import maps
+
+### Common Issues
+- Filename processing (extensions, special characters)
+- Asset pipeline compatibility (Sprockets vs Propshaft)
+- Module resolution and mapping
+- CDN integration and downloads
+- Security features (SRI hashes)
+
+## Example Commit Messages
+
+### Good Examples
+```
+Fix pin_all_from incorrectly removing "js" substring from filenames
+
+When using pin_all_from, filenames containing "js" as a substring
+(like "foo.jszip.js" or "bar.jsmin.js") were having the substring
+incorrectly removed, resulting in malformed module names like
+"foozip" and "barmin" instead of "foo.jszip" and "bar.jsmin".
+
+Fixed the logic to only remove the file extension from the end
+of the filename, preserving any "js" substrings that appear
+elsewhere in the name.
+
+Fixes #282
+```
+
+```
+Add integrity hash support for Propshaft assets
+
+Propshaft users can now enable automatic SRI hash calculation
+for local assets by calling enable_integrity! in importmap.rb.
+This provides the same security benefits as Sprockets users
+without requiring manual hash management.
+
+Closes #245
+```
+
+### What to Avoid
+```
+# Too vague
+Fix bug in map processing
+
+# Implementation-focused
+Change String#remove to String#chomp in module_name_from method
+
+# Missing context
+Update filename handling
+```
+
+## Instructions
+
+When generating a commit message:
+
+1. **Analyze the changes** to understand the problem being solved
+2. **Identify the user impact** - what behavior was broken/improved?
+3. **Write a clear subject line** focusing on the fix, not the implementation
+4. **Explain the problem** in the body - what was wrong and why it mattered
+5. **Describe the solution** in user terms, not code terms
+6. **Reference the issue** if applicable
+
+Focus on helping future developers understand why the change was needed and what problem it solves.
diff --git a/lib/importmap/map.rb b/lib/importmap/map.rb
@@ -302,7 +302,7 @@ def module_name_from(filename, mapping)
       # folder/index
       index_regex = /(?:\/|^)index$/
 
-      [ mapping.under, filename.to_s.remove(filename.extname).remove(index_regex).presence ].compact.join("/")
+      [ mapping.under, filename.to_s.chomp(filename.extname).remove(index_regex).presence ].compact.join("/")
     end
 
     def module_path_from(filename, mapping)
diff --git a/test/dummy/app/javascript/controllers/bar.jsmin.js b/test/dummy/app/javascript/controllers/bar.jsmin.js
@@ -0,0 +1 @@
+export function testJsmin() { return 'jsmin test'; }
diff --git a/test/dummy/app/javascript/controllers/foo.jszip.js b/test/dummy/app/javascript/controllers/foo.jszip.js
@@ -0,0 +1 @@
+export function testJszip() { return 'jszip test'; }
diff --git a/test/importmap_test.rb b/test/importmap_test.rb
@@ -134,6 +134,17 @@ def setup
     assert_match %r|assets/my_lib-.*\.js|, generate_importmap_json["imports"]["my_lib"]
   end
 
+  test "pin_all_from handles filenames with js substring correctly" do
+    assert_includes generate_importmap_json["imports"].keys, "controllers/foo.jszip"
+    assert_includes generate_importmap_json["imports"].keys, "controllers/bar.jsmin"
+
+    refute_includes generate_importmap_json["imports"].keys, "controllers/foozip"
+    refute_includes generate_importmap_json["imports"].keys, "controllers/barmin"
+
+    assert_match %r|assets/controllers/foo\.jszip-.*\.js|, generate_importmap_json["imports"]["controllers/foo.jszip"]
+    assert_match %r|assets/controllers/bar\.jsmin-.*\.js|, generate_importmap_json["imports"]["controllers/bar.jsmin"]
+  end
+
   test "importmap json includes integrity hashes from integrity: true" do
     importmap = Importmap::Map.new.tap do |map|
       map.enable_integrity!

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+export function testJsmin() { return 'jsmin test'; }`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+export function testJszip() { return 'jszip test'; }`