Skip to content

Commit 84311cc

Browse files
justin808claude
andcommitted
Fix CI failure and add bundler validation improvements
Critical Fixes: - Fixed RSpec test mutation issue by deep copying config with YAML.load(YAML.dump()) - Prevents test contamination from mutating original_config snapshot Improvements: 1. Added bundler validation with helpful error messages - Validates assets_bundler is 'webpack' or 'rspack' - Provides clear error with valid options 2. Added memoization to getBundler() - Caches bundler module for performance - Documented that config requires restart to change 3. Enhanced edge case test coverage (+3 tests, now 17 total) - Tests undefined bundler (defaults to webpack) - Tests invalid bundler (throws clear error) - Tests memoization (returns cached instance) 4. Improved SWC config documentation - Made React runtime comment version-agnostic - Added TODO for React 19+ migration consideration All tests passing: 17 tests across 4 suites. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent a32ebff commit 84311cc

File tree

4 files changed

+73
-9
lines changed

4 files changed

+73
-9
lines changed

client/__tests__/webpack/bundlerUtils.spec.js

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,4 +104,38 @@ describe('bundlerUtils', () => {
104104
expect(plugin.name).toBe('MockCssExtractRspackPlugin');
105105
});
106106
});
107+
108+
describe('Edge cases and error handling', () => {
109+
it('defaults to webpack when assets_bundler is undefined', () => {
110+
mockConfig.assets_bundler = undefined;
111+
jest.doMock('shakapacker', () => ({ config: mockConfig }));
112+
const utils = require('../../../config/webpack/bundlerUtils');
113+
114+
const bundler = utils.getBundler();
115+
116+
expect(bundler).toBeDefined();
117+
expect(bundler.ProvidePlugin.name).toBe('MockProvidePlugin');
118+
});
119+
120+
it('throws error for invalid bundler type', () => {
121+
mockConfig.assets_bundler = 'invalid-bundler';
122+
jest.doMock('shakapacker', () => ({ config: mockConfig }));
123+
const utils = require('../../../config/webpack/bundlerUtils');
124+
125+
expect(() => utils.getBundler()).toThrow('Invalid assets_bundler: "invalid-bundler"');
126+
expect(() => utils.getBundler()).toThrow('Must be one of: webpack, rspack');
127+
});
128+
129+
it('returns cached bundler on subsequent calls', () => {
130+
mockConfig.assets_bundler = 'webpack';
131+
jest.doMock('shakapacker', () => ({ config: mockConfig }));
132+
const utils = require('../../../config/webpack/bundlerUtils');
133+
134+
const bundler1 = utils.getBundler();
135+
const bundler2 = utils.getBundler();
136+
137+
// Should return same instance (memoized)
138+
expect(bundler1).toBe(bundler2);
139+
});
140+
});
107141
});

config/swc.config.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ const customConfig = {
1313
// Use classic runtime for SSR compatibility with React on Rails
1414
// This ensures React is explicitly imported in each component file, which
1515
// provides better compatibility with server-side rendering in Rails.
16-
// Note: React 19 supports automatic runtime with SSR, but classic runtime
17-
// is more explicit and avoids potential issues with different React versions.
16+
// Classic runtime is more explicit and works reliably across all React versions.
17+
// TODO: Consider switching to 'automatic' runtime when fully on React 19+
1818
runtime: 'classic',
1919
// Enable React Fast Refresh in development
2020
refresh: env.isDevelopment && env.runningWebpackDevServer,

config/webpack/bundlerUtils.js

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,45 @@
88

99
const { config } = require('shakapacker');
1010

11+
const VALID_BUNDLERS = ['webpack', 'rspack'];
12+
13+
// Cache for bundler module (config is read at startup and cannot change without restart)
14+
let _cachedBundler = null;
15+
let _cachedBundlerType = null;
16+
1117
/**
1218
* Gets the appropriate bundler module based on shakapacker.yml configuration.
1319
*
20+
* Note: The bundler configuration is read from shakapacker.yml at startup.
21+
* Changing the config requires restarting the Node process. This function
22+
* memoizes the result for performance.
23+
*
1424
* @returns {Object} Either webpack or @rspack/core module
25+
* @throws {Error} If assets_bundler is not 'webpack' or 'rspack'
1526
*/
1627
const getBundler = () => {
17-
return config.assets_bundler === 'rspack'
28+
// Return cached bundler if config hasn't changed
29+
if (_cachedBundler && _cachedBundlerType === config.assets_bundler) {
30+
return _cachedBundler;
31+
}
32+
33+
// Validate bundler configuration
34+
const bundlerType = config.assets_bundler || 'webpack'; // Default to webpack
35+
if (!VALID_BUNDLERS.includes(bundlerType)) {
36+
throw new Error(
37+
`Invalid assets_bundler: "${bundlerType}". ` +
38+
`Must be one of: ${VALID_BUNDLERS.join(', ')}. ` +
39+
`Check config/shakapacker.yml`,
40+
);
41+
}
42+
43+
// Load and cache the bundler module
44+
_cachedBundlerType = bundlerType;
45+
_cachedBundler = bundlerType === 'rspack'
1846
? require('@rspack/core')
1947
: require('webpack');
48+
49+
return _cachedBundler;
2050
};
2151

2252
/**

spec/webpack/bundler_switching_spec.rb

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414

1515
describe 'switching between webpack and rspack' do
1616
it 'successfully builds with webpack' do
17-
# Set bundler to webpack
18-
config = original_config
17+
# Set bundler to webpack (deep copy to avoid mutating original_config)
18+
config = YAML.load(YAML.dump(original_config))
1919
config['default']['assets_bundler'] = 'webpack'
2020
File.write(shakapacker_config_path, YAML.dump(config))
2121

@@ -28,8 +28,8 @@
2828
end
2929

3030
it 'successfully builds with rspack' do
31-
# Set bundler to rspack
32-
config = original_config
31+
# Set bundler to rspack (deep copy to avoid mutating original_config)
32+
config = YAML.load(YAML.dump(original_config))
3333
config['default']['assets_bundler'] = 'rspack'
3434
File.write(shakapacker_config_path, YAML.dump(config))
3535

@@ -45,8 +45,8 @@
4545
bundlers = %w[webpack rspack]
4646

4747
bundlers.each do |bundler|
48-
# Set bundler
49-
config = original_config
48+
# Set bundler (deep copy to avoid mutating original_config)
49+
config = YAML.load(YAML.dump(original_config))
5050
config['default']['assets_bundler'] = bundler
5151
File.write(shakapacker_config_path, YAML.dump(config))
5252

0 commit comments

Comments
 (0)