plack / Plack

PSGI toolkit and server adapters
http://plackperl.org/
Other
486 stars 214 forks source link

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

Open tsibley opened 12 years ago

tsibley commented 12 years ago

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]!;
miyagawa commented 12 years ago

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 12 years ago

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.

fsitedev commented 12 years ago

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

bockscar commented 11 years ago

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]!;
}
miyagawa commented 11 years ago

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'?

bockscar commented 11 years ago

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]!;
}
bockscar commented 11 years ago

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

miyagawa commented 11 years ago

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.

bockscar commented 11 years ago

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.

miyagawa commented 11 years ago

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.

bockscar commented 11 years ago

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.

miyagawa commented 11 years ago

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.