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 spring watch #82

Closed
wants to merge 7 commits into from
Closed

Add spring watch #82

wants to merge 7 commits into from

Conversation

professor
Copy link
Contributor

@professor professor commented Feb 2, 2024

When a pack moves, we want spring to update the loaded classes so that packwerk prints correct results

Screenshot 2024-02-13 at 13 40 43

Fixes #81

puts "Hello Db Blasters!"
Packs::Specification::Configuration.fetch.pack_paths.each do |dir|
puts "#{dir}"
Dir["#{dir}/package.yml"].each { |f|
Copy link
Contributor

Choose a reason for hiding this comment

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

why would you do a glob here (Dir[])? Isn't dir the exact path? In which case you'll want File.join(dir, "package.yml") or something like that

But I'm probably misunderstanding pack_paths...

Copy link
Contributor

@perryqh perryqh Feb 13, 2024

Choose a reason for hiding this comment

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

Great question!

Example pack_paths:

  • packs/*
  • packs/*/*

We need Dir[] to get the full paths to package.yml files

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think pack_paths is easy to misunderstand; it's more like pack_path_specs or pack_path_globs, but that's out of scope here

professor and others added 2 commits February 9, 2024 14:12
When a pack moves, we want spring to update the loaded classes
so that packwerk prints correct results

Co-authored-by: Teal Stannard <[email protected]>
Co-authored-by: Ashley Willard <[email protected]>
Co-authored-by: Joey Schoblaska <[email protected]>
Co-authored-by: Ashley Willard <[email protected]>
Co-authored-by: Joey Schoblaska <[email protected]>
Co-authored-by: Perry Hertler <[email protected]>
@perryqh perryqh force-pushed the ts/add-spring-watch branch from acd06a7 to 35e6731 Compare February 9, 2024 20:12
lib/packs/rails/railtie.rb Outdated Show resolved Hide resolved
perryqh and others added 3 commits February 13, 2024 13:42
@perryqh perryqh enabled auto-merge (squash) February 17, 2024 00:51
@exterm
Copy link
Contributor

exterm commented Feb 17, 2024

Very excited for this to land

@exterm
Copy link
Contributor

exterm commented Feb 19, 2024

One note: this doesn't seem to cover the case where an autoload path is added and none of the previously registered package.yml files is changed.

I just added an initializer to a pack and had to stop spring manually.

@perryqh
Copy link
Contributor

perryqh commented Feb 23, 2024

One note: this doesn't seem to cover the case where an autoload path is added and none of the previously registered package.yml files is changed.

I just added an initializer to a pack and had to stop spring manually.

Thanks for the comment!

We want to clarify the expectation. Did you add a new autoload path or a new file in the autoload path?

We noticed that Spring has a limitation on only watching files it knows about when initialized. New files, including ruby files, will not cause Spring to reload.

Co-authored-by: Ashley Willard <[email protected]>
Co-authored-by: Joey Schoblaska <[email protected]>
@perryqh perryqh force-pushed the ts/add-spring-watch branch from f115e2a to 99e9669 Compare February 23, 2024 21:47
@exterm
Copy link
Contributor

exterm commented Feb 24, 2024

We noticed that Spring has a limitation on only watching files it knows about when initialized. New files, including ruby files, will not cause Spring to reload.

Mmmh, was that with or without https://github.com/rails/spring-watcher-listen ? Listen, according to its documentation, can detect added files in watched directories.

Detects file modification, addition and removal.

https://github.com/guard/listen?tab=readme-ov-file#features

And here's the code in spring-watcher-listen that listens to additions: https://github.com/rails/spring-watcher-listen/blob/996d29e8f9b38fb642dd6b496ed600e941bd7de1/lib/spring/watcher/listen.rb#L62 ... I wonder if that's only for directories outside of Rails.root? Curious.

Did you add a new autoload path or a new file in the autoload path?

If I remember correctly, I added a pack with new ruby files and didn't modify any existing package.yml files. In general - why would we expect this mechanism to be able to pick up new autoload paths from newly added packages? We only respect package.yml files that were present at application startup, no?

I wonder if we could listen to additions of package.yml files... but it seems that would require a custom spring watcher, or modifications to spring-watcher-listen.

@professor
Copy link
Contributor Author

Replaced by #86

@professor professor closed this Mar 8, 2024
auto-merge was automatically disabled March 8, 2024 21:16

Pull request was closed

@exterm
Copy link
Contributor

exterm commented Mar 10, 2024

The other PR says this was closed because it "started to need more and more gem dependencies". I don't see those in the diff. Can you expand on the problem you encountered?

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.

Trouble with spring after moving / renaming packages?
6 participants