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

Already on GitHub? Sign in to your account

Plack::Loader::Restarter won't restart when a sass partial is changed #260

Open
tsibley opened this Issue Dec 13, 2011 · 12 comments

Comments

Projects
None yet
4 participants

tsibley commented Dec 13, 2011

Sass partials start with an underscore, which is explicitly checked in valid_file.

Our use case: plackup -p 8888 -e "BEGIN { system('./bin/generate-css'); }" -R static/style,static/js app.psgi

Before we switched to sass partials, this rocked our dev cycles by restarting on any Perl, CSS, or JS change (and recompiling our CSS with Compass). More about partials: http://sass-lang.com/docs/yardoc/file.SASS_REFERENCE.html#partials

The offending regex is:

$file->{path} !~ m![/\\][\._]|\.bak$|~$|_flymake\.p[lm]!;
Owner

miyagawa commented Mar 8, 2012

I'm not sure what would be the best, but i think we should be able to let users customize the regexp path to look for and ignore changes.

tsibley commented Mar 8, 2012

On 03/08/2012 02:05 PM, Tatsuhiko Miyagawa wrote:

I'm not sure what would be the best, but i think we should be able to
let users customize the regexp path to look for and ignore changes.

Letting devs customize that regexp easily would be great and hopefully
simpler than trying to please everyone with a single regex.

We have much more global problem with this regexp because of dir named ".www" in project path.
So, none of the modules never restarted

Wow, spent a long time on this and finally drilled down to it from my Catalyst app. Prob is the Plack::Loader::Restarter->valid_file method. It is called to check whether a change in a particular file path merits a restart.

I'm on a Mac and it's matching every path returned by Mac::FSEvents. This is a lame fix, but it works great for me and allows a file change in prescribed path(s) to trigger a restart as expected:

sub valid_file {
    my($self, $file) = @_;
    return 1;
#    $file->{path} !~ m![/\\][\._]|\.bak$|~$|_flymake\.p[lm]!;
}
Owner

miyagawa commented Oct 18, 2012

I'm on a Mac and it's matching every path returned by Mac::FSEvents.

I'm on a Mac too and i'm curious what you mean by that it 'matches every path'?

I didn't drill down too deep once I found it, but it appears Mac::FSEvents returns fully qualified baths, i.e., they all start with a slash. But this regex is trying to ensure no slashes (forward or backward) are in the path. Because of that, it does NOT match anything and returns 0 for every single changed file that is passed to it.

I'm in the process of doing a slightly more sophisticated check, still in progress, something like code below because I only want to restart for Perl files (suffixed with .pl and .pm):

sub valid_file {
    my($self, $file) = @_;
    return 1 if $file->{path} =~ m~\.p[lm]$~;
    # $file->{path} !~ m![/\\][\._]|\.bak$|~$|_flymake\.p[lm]!;
}

Ah, I should've said, I totally agree with your comment above that having option to let users customize regex would be really useful.

Owner

miyagawa commented Oct 19, 2012

But this regex is trying to ensure no slashes (forward or backward) are in the path.

No it's not. You are reading the regular expression wrong. Dot or underscore (._) should follow after the slashes to actually match the file (to ignore). This is to ignore dot files.

You're exactly right. I didn't look closely enough.

But that regex still messed me up for a completely different reason it turns out. Here's the path where all my stuff lives:

/Dropbox/_work_shared/stuff

So that underscore in regex catches a directory starting with an underscore. Maybe that's desired behavior. Definitely threw me for a loop.

This fix addressed my perhaps unusual case:

$file->{path} !~ m!^[/\\][\._]|\.bak$|~$|_flymake\.p[lm]!;

But even then, I still want to customize the restart criteria for my needs. In my case, I don't need a restart because a GIF changes; I only want a restart in very specific circumstances.

Owner

miyagawa commented Oct 19, 2012

Yeah that's the same as the other guy on this thread who uses the directory named ".www". Maybe it should not match the directory (not files) that begins with ._ as a default, and then an ability to override that anyway.

This fix addressed my perhaps unusual case:

I don't think so - well, it will then only match the files/directories that begins with ._ in root directory (/) that's completely unlikely.

Yeah, that'd be cool and it'd meet a broader scope of use cases.

Was also hoping to customize latency figure that you pass through to Filesys::Notify::Simple, but it's hardcoded in the latter. Oh well.

Thanks for staying in tune and for all the great work. I'm new to this module and Plack overall. Awesome stuff.

Owner

miyagawa commented Oct 19, 2012

I pushed a fix to match only with filenames and not directories, so directories that begin with . or _ for some reason will not be automatically ignored anymore.

@miyagawa miyagawa added a commit that referenced this issue Oct 21, 2012

@miyagawa miyagawa Checking in changes prior to tagging of version 1.0007.
Changelog diff is:

diff --git a/Changes b/Changes
index 13330ff..76d3cd1 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,10 @@
 Go to http://github.com/plack/Plack/issues for the roadmap and known issues.

+1.0007  Sat Oct 20 23:20:20 PDT 2012
+    [IMPROVEMENTS]
+        - Fix test failures with HTTP::Message 6.06. #345
+        - relaxed plackup -R ignore files and directoris. #260
+
 1.0006  Thu Oct 18 16:06:15 PDT 2012
     [INCOMPATIBLE CHANGES]
         - plackup foo.psgi will not search the file in @INC anymore before the current directory
3958aa4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment