Allow loading webpack.config.ts if present #524
Conversation
WalkthroughThe pull request introduces significant updates to the Shakapacker project, including changes to the changelog, README, and core functionality. Key modifications involve transitioning from Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jdelStrother
force-pushed
the
webpack-ts
branch
2 times, most recently
from
7b3161f
to
ccc4706
Compare
jdelStrother
force-pushed
the
webpack-ts
branch
from
ccc4706
to
7c11b4b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
spec/shakapacker/webpack_runner_spec.rb (1)
58-67: LGTM with suggestions for additional test coverage.
The test case correctly verifies the basic TypeScript config loading capability. However, consider enhancing the test suite with additional cases:
Consider adding these test cases for more robust coverage:
it "raises error when webpack.config.ts exists but typescript deps are missing" do
ts_config = "#{test_app_path}/config/webpack/webpack.config.ts"
FileUtils.touch(ts_config)
# Simulate missing typescript dependencies
allow(PackageJson).to receive(:list_dependencies).and_return([])
expect {
package_json.manager.native_exec_command("webpack", ["--config", ts_config])
}.to raise_error(/Please add typescript and ts-node to your package.json/)
ensure
FileUtils.rm(ts_config)
end
it "prioritizes webpack.config.js over webpack.config.ts when both exist" do
ts_config = "#{test_app_path}/config/webpack/webpack.config.ts"
js_config = "#{test_app_path}/config/webpack/webpack.config.js"
FileUtils.touch([ts_config, js_config])
cmd = package_json.manager.native_exec_command("webpack", ["--config", js_config])
verify_command(cmd)
ensure
FileUtils.rm([ts_config, js_config])
endCHANGELOG.md (1)
15-15: Use consistent list style markers
The unordered list uses a dash (-) while other lists in the file use asterisks (*). Consider updating for consistency:
- - Allow configuring webpack from a Typescript file (`config/webpack/webpack.config.ts`). [PR 524](https://github.com/shakacode/shakapacker/pull/524) by [jdelStrother](https://github.com/jdelStrother).
+ * Allow configuring webpack from a Typescript file (`config/webpack/webpack.config.ts`). [PR 524](https://github.com/shakacode/shakapacker/pull/524) by [jdelStrother](https://github.com/jdelStrother).🧰 Tools
🪛 Markdownlint
15-15: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
README.md (1)
684-694: Consider enhancing the TypeScript configuration example.
While the TypeScript configuration is correct, it could be improved with type annotations and additional context.
Consider this enhanced version:
// config/webpack/webpack.config.ts
import { generateWebpackConfig } from "shakapacker";
import ForkTSCheckerWebpackPlugin from "fork-ts-checker-webpack-plugin";
+import type { Configuration } from 'webpack';
-const config = generateWebpackConfig({
+const customConfig: Partial<Configuration> = {
plugins: [new ForkTSCheckerWebpackPlugin()],
-});
+};
+
+const config: Configuration = generateWebpackConfig(customConfig);
export default config;Also consider adding a note about ensuring tsconfig.json includes the webpack config directory in its compilation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- lib/shakapacker/runner.rb (1 hunks)
- spec/shakapacker/webpack_runner_spec.rb (1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
15-15: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
🔇 Additional comments (5)
lib/shakapacker/runner.rb (3)
19-19: LGTM: Clean refactor of webpack config initialization
The change elegantly delegates the webpack configuration discovery to a dedicated method, improving code organization and extensibility.
28-29: LGTM: Proper method organization
The private method section follows Ruby conventions and maintains clean code organization.
30-38: 🛠️ Refactor suggestion
Enhance error handling and improve code clarity
While the implementation is functional, there are several opportunities for improvement:
- The error message only mentions the JavaScript config file, which could be misleading since TypeScript is also supported.
- The single-letter parameter
fin the block could be more descriptive. - The error message should mention the TypeScript dependencies requirement.
- Consider adding file permission checks for better error handling.
Here's a suggested improvement:
def find_webpack_config
possible_paths = %w[ts js].map do |ext|
File.join(@app_path, "config/webpack/webpack.config.#{ext}")
end
- path = possible_paths.find { |f| File.exist?(f) }
+ path = possible_paths.find { |config_path| File.exist?(config_path) && File.readable?(config_path) }
unless path
- $stderr.puts "webpack config #{possible_paths.last} not found, please run 'bundle exec rails shakapacker:install' to install Shakapacker with default configs or add the missing config file for your custom environment."
+ $stderr.puts "No webpack config found. Tried: #{possible_paths.join(', ')}\n" \
+ "Please either:\n" \
+ "1. Run 'bundle exec rails shakapacker:install' to install Shakapacker with default configs\n" \
+ "2. Add webpack.config.js for JavaScript configuration\n" \
+ "3. Add webpack.config.ts and ensure ts-node/typescript are in package.json for TypeScript configuration"
exit!
end
path
endLet's verify if the repository has appropriate documentation about the TypeScript requirements:
spec/shakapacker/webpack_runner_spec.rb (1)
58-67: Verify TypeScript dependency handling in the implementation.
The PR description mentions that users need to add ts-node and typescript to their package.json. However, the test doesn't verify if the implementation actually checks for these dependencies.
Let's verify if the dependency check is implemented:
✅ Verification successful
Looking at the implementation in lib/shakapacker/runner.rb, I can now generate a final response:
Implementation correctly handles TypeScript config without explicit dependency checks
The implementation in lib/shakapacker/runner.rb shows that the code automatically looks for both .ts and .js webpack configurations without explicitly checking for TypeScript dependencies. This is actually the correct approach because:
- The
find_webpack_configmethod tries TypeScript config first (webpack.config.ts), falling back to JavaScript if not found - If neither config exists, it provides a helpful error message
- The actual TypeScript execution is handled by Node.js and requires the dependencies to be present in
package.json, which is the correct place for these checks
The current implementation follows the principle of "fail fast" - if TypeScript dependencies are missing, the webpack execution itself will fail with a clear Node.js error about missing dependencies, which is more appropriate than adding redundant dependency checks in the Ruby layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for typescript dependency checks in the codebase
rg -l "ts-node|typescript" --type ruby
Length of output: 40
Script:
#!/bin/bash
# Let's check the implementation files for webpack configuration handling
rg -A 10 "webpack.config" --type ruby
Length of output: 11415
README.md (1)
678-682: LGTM: Package installation instructions are clear and complete.
The instructions correctly list all necessary packages for TypeScript webpack configuration.
Summary
This allows configuring webpack from Typescript in
config/webpack/webpack.config.tsif it's present.Further instructions on configuring webpack with typescript are here, but if you've added ts-node & typescript to your package.json that should be all that's needed for webpack.config.ts to just work.
Pull Request checklist
Summary by CodeRabbit
New Features
webpack.config.ts).Bug Fixes
Documentation
Tests