Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Pack if Missing Generated Pack for Registered Component #1506

Merged
merged 13 commits into from
Jan 28, 2023

Conversation

pulkitkkr
Copy link
Contributor

@pulkitkkr pulkitkkr commented Jan 16, 2023

Problem

Currently, The Generate Pack Script handles the on-demand generation of packs only for existing components by checking m_time to verify if the generated packs are stale. If a user creates a new Component under components_subdirectory, It's not tracked by stale_generated_component_packs;

Solution and Changes in PR

This PR updates the FS-based Pack generation feature by adding a check for missing_packs. The system will generate a new pack when either the components are stale, or a new component is created.


This change is Reviewable

@pulkitkkr pulkitkkr force-pushed the Generate_Packs_When_Generated_Pack_Is_Missing branch 2 times, most recently from 18f8aa8 to ea9864a Compare January 25, 2023 07:54
@pulkitkkr pulkitkkr requested a review from justin808 January 26, 2023 09:22
Copy link
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.

Looks OK, but where are the tests that break without the fix and pass with the fix?

@pulkitkkr
Copy link
Contributor Author

Looks OK, but where are the tests that break without the fix and pass with the fix?

This is a feature that Get's Triggered Only If someone creates a new component while Rails Server is still running. I don't know if we have test cases to cover such scenarios or if we should plan to add them.

Copy link
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.

where are the tests?

@@ -556,8 +556,7 @@ def initialize_redux_stores

def raise_generated_missing_pack_warning(component_name)
msg = <<~MSG
**ERROR** ReactOnRails: Generated missing pack for Component: #{component_name}. Please refresh the webpage \
once webpack has finished generating the bundles. If the problem persists
**ERROR** ReactOnRails: Generated missing pack for Component: #{component_name}. Please restart the webpack dev server to ensure webpack generates the chunks corresponding to #{component_name} component. If the problem persists
Copy link
Member

Choose a reason for hiding this comment

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

or webpack in watch mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can not run webpack in watch mode as creating a new Component makes a new entry point. To accommodate this new entry point, the server needs to get restarted as setEntry to webpack is configured on initialization of webpack.

Copy link
Member

Choose a reason for hiding this comment

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

that's my point

the error says webpack dev server only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


is_generated_directory_present = Dir.exist?(generated_packs_directory_path)

return if is_generated_directory_present && webpack_assets_status_checker.stale_generated_component_packs.empty?
return if is_generated_directory_present && stale_or_missing_packs.empty?
Copy link
Member

Choose a reason for hiding this comment

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

is the stale_or_missing_packs list used to optimize what's generated?

Copy link
Member

Choose a reason for hiding this comment

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

If not, then why keep going after one is detected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This optimization should be a separate PR as it requires additional testing and might introduce new bugs if not done correctly. Considering the time sensitivity of this work, I would suggest creating a new Issue for optimization, which we can work on later.

Copy link
Member

Choose a reason for hiding this comment

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

then exit early on the first found stale or missing entry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will use this list later to generate packs just for them. I can exit it early by making the stale_or_missing_packs return boolean. But since we are going to use it very soon, I suggest keeping it this way for now.

@pulkitkkr pulkitkkr requested a review from justin808 January 27, 2023 07:40
@pulkitkkr pulkitkkr force-pushed the Generate_Packs_When_Generated_Pack_Is_Missing branch 6 times, most recently from ddf4b6b to 4f0361d Compare January 27, 2023 10:36
@pulkitkkr pulkitkkr force-pushed the Generate_Packs_When_Generated_Pack_Is_Missing branch from f145bb4 to a48c46d Compare January 28, 2023 07:44
@pulkitkkr pulkitkkr requested a review from justin808 January 28, 2023 07:45
@pulkitkkr pulkitkkr force-pushed the Generate_Packs_When_Generated_Pack_Is_Missing branch from a48c46d to 6ae3f54 Compare January 28, 2023 07:46
msg = <<~MSG
**ERROR** ReactOnRails: Generated missing pack for Component: #{component_name}. Please refresh the webpage \
once webpack has finished generating the bundles. If the problem persists
**ERROR** ReactOnRails: Generated missing pack for Component: #{component_name}. Please restart the webpack dev server or webpack in watch mode, to ensure webpack generates the chunks corresponding to #{component_name} component. If the problem persists
Copy link
Member

Choose a reason for hiding this comment

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

should this error message be detected in some tests?

@justin808 justin808 merged commit 41e946d into master Jan 28, 2023
@justin808 justin808 deleted the Generate_Packs_When_Generated_Pack_Is_Missing branch January 28, 2023 08:12
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