Skip to content

Typo in rescue for server rendering#963

Merged
justin808 merged 4 commits intoshakacode:masterfrom
AnatoliiD:patch-1
Oct 28, 2017
Merged

Typo in rescue for server rendering#963
justin808 merged 4 commits intoshakacode:masterfrom
AnatoliiD:patch-1

Conversation

@AnatoliiD
Copy link
Copy Markdown

@AnatoliiD AnatoliiD commented Oct 23, 2017

This change is Reviewable

@justin808
Copy link
Copy Markdown
Member

@railsme this is GREAT!

Can you please add a CHANGELOG.md entry and I'll merge this.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@AnatoliiD
Copy link
Copy Markdown
Author

done 👌


Comments from Reviewable

@justin808
Copy link
Copy Markdown
Member

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CHANGELOG.md, line 11 at r2 (raw file):

*Please add entries here for your pull requests.*
#### Fixed
- rubocop disabling comments in rescue statements [PR 963](https://github.com/shakacode/react_on_rails/pull/963) by [railsme](https://github.com/railsme).

We can make this description a bit more detailed, along the lines of the rubocop comment broke...and this fixed ...

plus, please rebase your change on master

I can't merge right now due to a conflict...


Comments from Reviewable

@AnatoliiD
Copy link
Copy Markdown
Author

Should be better now 😃

@justin808
Copy link
Copy Markdown
Member

@railsme We're not passing Travis

@AnatoliiD
Copy link
Copy Markdown
Author

@justin808 #899 broke the build. Should I try to fix it in my PR or wait for fix from the author? 🤔

@justin808
Copy link
Copy Markdown
Member

If you can fix it, I’d be thrilled. In this pr is fine.

@AnatoliiD
Copy link
Copy Markdown
Author

@justin808, I've found that we are using empty strings as default values for not set config options.
I have doubts about checking directory in LocalesToJs because we have similar checks here.
I suppose that #899 should be reverted or totally replaced in separate PR because this is not part of the current fix.

@AnatoliiD
Copy link
Copy Markdown
Author

AnatoliiD commented Oct 28, 2017

@justin808
Please, check and accept #967 😄
I will rebase current branch on top of master.

@justin808
Copy link
Copy Markdown
Member

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit 3ce836b into shakacode:master Oct 28, 2017
ihabadham added a commit that referenced this pull request Mar 9, 2026
…2580)

## Summary

- Reverts #2561 (squash commit 769d120), which was merged after #2568
already landed on master
- #2568 +
[shakacode/shakapacker#963](shakacode/shakapacker#963)
fix the root cause of #2438#2561's YAML-parsing fallback is redundant
- CHANGELOG entry updated to reference #2568 only

## What's removed

All of #2561's additions on top of #2568:
- `extract_precompile_hook_from_yaml` /
`extract_precompile_hook_from_shakapacker_config` split
- `extract_script_from_command` interpreter-prefix stripping
- `extract_hash_for_environment` / `current_shakapacker_environment`
helpers
- `SHAKAPACKER_CONFIG_PATH` constant, `require "erb"`, `require "yaml"`
- `rubocop:disable Metrics/ModuleLength`
- `GENERATE_PACKS_PATTERN` expansion for `generate_packs_if_needed`
- 72 lines of #2561-specific specs

## What remains (from #2568 + shakapacker#963)

- `project_root` helper (Rails.root → BUNDLE_GEMFILE → Dir.pwd fallback)
- `resolve_hook_script_path` using `project_root`
- `require "pathname"`
- 3 project_root specs
- Shakapacker `Env#current` guard (separate repo, already merged)

## Why

#2568 was designed to supersede #2561 — its PR description says so
explicitly. Both were merged, resulting in unnecessary complexity (YAML
config re-parsing, ERB evaluation, environment fallback chain) that
duplicates what Shakapacker already handles after #963.

## Test plan

- `bundle exec rspec
react_on_rails/spec/react_on_rails/packer_utils_spec.rb`
- `bundle exec rubocop react_on_rails/lib/react_on_rails/packer_utils.rb
react_on_rails/spec/react_on_rails/packer_utils_spec.rb`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved bin/dev startup reliability with enhanced hook script path
resolution during early startup when Rails.root is unavailable.
  * Removed YAML configuration dependency, streamlining hook detection.
* Added validation checks for hook script existence to prevent runtime
errors.
  * Simplified hook script pattern matching for better clarity.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants