Skip to content

Update Helper to Check for Bundle with correct Extension#1511

Merged
justin808 merged 6 commits intomasterfrom
ROR1510-bug-fix-missing-pack-warning
Jan 28, 2023
Merged

Update Helper to Check for Bundle with correct Extension#1511
justin808 merged 6 commits intomasterfrom
ROR1510-bug-fix-missing-pack-warning

Conversation

@pulkitkkr
Copy link
Copy Markdown
Contributor

@pulkitkkr pulkitkkr commented Jan 26, 2023

Summary

This PR fixes the bug as described in the Issue: #1510


This change is Reviewable

@pulkitkkr pulkitkkr force-pushed the ROR1510-bug-fix-missing-pack-warning branch 2 times, most recently from ea1769b to f57a4e3 Compare January 26, 2023 08:46
@pulkitkkr pulkitkkr force-pushed the ROR1510-bug-fix-missing-pack-warning branch from f57a4e3 to 51a86c3 Compare January 26, 2023 09:12
@pulkitkkr pulkitkkr force-pushed the ROR1510-bug-fix-missing-pack-warning branch from 51a86c3 to 5fb4f35 Compare January 26, 2023 09:14
Comment thread lib/react_on_rails/helper.rb Outdated
def generated_components_pack(component_name)
"#{ReactOnRails::WebpackerUtils.webpacker_source_entry_path}/generated/#{component_name}"
def generated_components_pack_path(component_name)
extension = PacksGenerator.generated_pack_extension
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this customizable? Why not always .js?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not configurable! It's always js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why not a constant?

Comment thread CHANGELOG.md Outdated

*Please add entries here for your pull requests that are not yet released.*
- Added `./bin/dev` and `./bin/dev-static` executables to ease and standardize running the dev server. [PR 1491](https://github.com/shakacode/react_on_rails/pull/1491) by [ahangarha](https://github.com/ahangarha)
- Fixed pack not found warning while using `react_component` and `react_component_hash` helpers, even when corresponding chunks are present in `manifest` file [PR 1511](https://github.com/shakacode/react_on_rails/pull/1511) by [pulkitkkr](https://github.com/pulkitkkr)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the manifest have to do with the changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it to:

Fixed pack not found warning while using `react_component` and `react_component_hash` helpers, even when corresponding chunks are present.

@pulkitkkr pulkitkkr requested a review from justin808 January 26, 2023 10:17
Copy link
Copy Markdown
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no tests?

Comment thread lib/react_on_rails/helper.rb Outdated
def generated_components_pack(component_name)
"#{ReactOnRails::WebpackerUtils.webpacker_source_entry_path}/generated/#{component_name}"
def generated_components_pack_path(component_name)
extension = PacksGenerator.generated_pack_extension
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why not a constant?

@pulkitkkr
Copy link
Copy Markdown
Contributor Author

Added Tests

Copy link
Copy Markdown
Contributor Author

@pulkitkkr pulkitkkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @Judahmeek and @justin808)


lib/react_on_rails/helper.rb line 112 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Then why not a constant?

Done.

@pulkitkkr pulkitkkr requested a review from justin808 January 27, 2023 07:32
@justin808 justin808 merged commit fb3210c into master Jan 28, 2023
@justin808 justin808 deleted the ROR1510-bug-fix-missing-pack-warning branch January 28, 2023 07:04

def generated_components_pack(component_name)
"#{ReactOnRails::WebpackerUtils.webpacker_source_entry_path}/generated/#{component_name}"
def generated_components_pack_path(component_name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be part of the helper? or not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be a private member

@@ -94,8 +94,7 @@ def react_component(component_name, options = {})
end

def load_pack_for_component(component_name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this part of the public helper?

there's no doc for this method load_pack_for_component
or generated_components_pack_path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be private member as it is used internally by react_component and react_component_hash

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