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

Add an option to ignore unnecessary directories such as node_modules/ #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mktakuya
Copy link

@mktakuya mktakuya commented Dec 30, 2024

This Pull Request introduces an ignore option to the configuration settings.

Motivation / Background

In my hobby application, I encountered an error caused by the rails_live_reload gem monitoring JavaScript files under the node_modules/ directory.
After investigating the issue, I found that properly configuring the ignore option in the Listen.to method, which is used internally by this gem, resolves the problem.
To help other developers address similar issues independently, I added the ignore option to the gem's configuration settings.

Related issue: #38

@mktakuya mktakuya changed the title Add an option to ignore unnecessary directories such as node_modules/ and tmp/ Add an option to ignore unnecessary directories such as node_modules/ Dec 30, 2024
@mktakuya mktakuya force-pushed the feature/ignore-patterns branch from 0963181 to 95d7ac7 Compare December 30, 2024 16:10
Comment on lines 41 to 44
@ignore_patterns = [
%r{node_modules/}
]
@default_ignore_patterns_changed = false
Copy link

Choose a reason for hiding this comment

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

Personally, I would keep this simple and backwards compatible with RLR as it exists today, and not by default ignore node_modules which may break some people's existing expectations for their project (especially not without updating documentation to indicate that node_modules is ignored). Rather, I think modifying the generator as you've done is plenty.

It also lets you simplify things by having @ignore_patterns = [] in the initializer without needing the @default_ignore_patterns_changed state.

Copy link
Author

@mktakuya mktakuya Jan 6, 2025

Choose a reason for hiding this comment

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

@dymk
Thank you for your feedback! 🙏
I’ve made the suggested changes in 3c55416 :

  • Updated the implementation to avoid ignoring node_modules/ by default, maintaining compatibility with existing projects.
  • Simplified the initializer by removing the @default_ignore_patterns_changed state.

@mktakuya mktakuya force-pushed the feature/ignore-patterns branch 2 times, most recently from 32a8856 to 3c55416 Compare January 6, 2025 14:32
@mktakuya mktakuya requested a review from dymk January 6, 2025 14:38
The `Listen.to` method accepts an `ignore` option, allowing the listener to ignore paths matching specified patterns.

In this commit, the `ignore` option passed to the Listen.to method is made configurable through a configuration file.
@mktakuya mktakuya force-pushed the feature/ignore-patterns branch from 3c55416 to a8955fe Compare January 11, 2025 15:27
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