Hello. I create small patch to enhance static files serving functionality. During development process its important to refresh pages quickly. Conditional HTTP requests is the good choise. Seems like working, but needs more testing :) --- Dancer/Renderer.pm.orig 2011-02-10 19:31:07.000000000 +0300 +++ Dancer/Renderer.pm 2011-02-10 20:28:46.000000000 +0300 @@ -3,7 +3,9 @@ use strict; use warnings; use Carp; +use File::stat; use HTTP::Headers; +use POSIX qw( strftime ); use Dancer::Route; use Dancer::HTTP; use Dancer::Cookie; @@ -164,10 +166,16 @@ my $fh = open_file('<', $static_file); binmode $fh; - Dancer::Response->status($status); + # http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html + # Wed, 15 Nov 1995 04:58:08 GMT + my $lmod = strftime '%a, %d %b %Y %H:%M:%S GMT', gmtime(stat($fh)->mtime); + my $imod = Dancer::SharedData->request->header('If-Modified-Since') || ''; + + Dancer::Response->status( $lmod eq $imod ? 304 : $status ); Dancer::Response->content_type( get_mime_type($static_file)); - Dancer::Response::set_current_content($fh); + Dancer::Response::set_current_content($fh) unless $lmod eq $imod; + Dancer::Response->header('Last-Modified' => $lmod ); return Dancer::Response->current; } return; -- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
On 10/02/11 17:36, Oleg A. Mamontov wrote:
+ my $lmod = strftime '%a, %d %b %Y %H:%M:%S GMT', gmtime(stat($fh)->mtime); + my $imod = Dancer::SharedData->request->header('If-Modified-Since') || ''; + + Dancer::Response->status( $lmod eq $imod ? 304 : $status ); ... + Dancer::Response::set_current_content($fh) unless $lmod eq $imod;
Should this not be "ge" rather than "eq"? -- Richard Huxton Archonet Ltd
On Fri, 2011-02-11 at 12:38 +0000, Richard Huxton wrote:
On 10/02/11 17:36, Oleg A. Mamontov wrote:
+ my $lmod = strftime '%a, %d %b %Y %H:%M:%S GMT', gmtime(stat($fh)->mtime); + my $imod = Dancer::SharedData->request->header('If-Modified-Since') || ''; + + Dancer::Response->status( $lmod eq $imod ? 304 : $status ); ... + Dancer::Response::set_current_content($fh) unless $lmod eq $imod;
Should this not be "ge" rather than "eq"?
I don't think so; if it was last modified after the "If-Modified-Since" datetime, you want to send the response, rather than a 304 Not Modified response. You could use "le" instead, but there shouldn't be any situation where the file was last modified before the If-Modified-Since header, unless the client modified the header perhaps. -- David Precious ("bigpresh") http://www.preshweb.co.uk/ "Programming is like sex. One mistake and you have to support it for the rest of your life". (Michael Sinz)
On Feb 11, 2011, at 3:38 PM, Richard Huxton wrote:
On 10/02/11 17:36, Oleg A. Mamontov wrote:
+ my $lmod = strftime '%a, %d %b %Y %H:%M:%S GMT', gmtime(stat($fh)->mtime); + my $imod = Dancer::SharedData->request->header('If-Modified-Since') || ''; + + Dancer::Response->status( $lmod eq $imod ? 304 : $status ); ... + Dancer::Response::set_current_content($fh) unless $lmod eq $imod;
Should this not be "ge" rather than "eq"?
No, for a developer environment (in which this method of serving static content is used) simple comparison of the strings equivalence is quite enough. Otherwise we'll have to parse request header, this operation is more difficult and will create extra dependencies.
-- Richard Huxton Archonet Ltd _______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
Le 10/02/2011 18:36, Oleg A. Mamontov a écrit :
Hello.
I create small patch to enhance static files serving functionality. During development process its important to refresh pages quickly. Conditional HTTP requests is the good choise. Seems like working, but needs more testing :)
I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware. Any specific reason not to use the appropriate middleware? Regards, -- Alexis Sukrieh
On Feb 11, 2011, at 4:54 PM, Alexis Sukrieh wrote:
Le 10/02/2011 18:36, Oleg A. Mamontov a écrit :
Hello.
I create small patch to enhance static files serving functionality. During development process its important to refresh pages quickly. Conditional HTTP requests is the good choise. Seems like working, but needs more testing :)
I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware.
Any specific reason not to use the appropriate middleware?
Nobody use `./bin/app.pl` for development (with no Plack) ?
Regards,
-- Alexis Sukrieh _______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
On Fri, Feb 11, 2011 at 3:04 PM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
On Feb 11, 2011, at 4:54 PM, Alexis Sukrieh wrote:
Le 10/02/2011 18:36, Oleg A. Mamontov a écrit :
Hello.
I create small patch to enhance static files serving functionality. During development process its important to refresh pages quickly. Conditional HTTP requests is the good choise. Seems like working, but needs more testing :)
I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware.
This is clearly a Plack solution. If you really need, you can maybe create a plugin for this, as soon as we add a new hook.
Any specific reason not to use the appropriate middleware?
Nobody use `./bin/app.pl` for development (with no Plack) ?
Regards,
-- Alexis Sukrieh _______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov
mailto: oleg@mamontov.net
jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
_______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- franck cuny http://lumberjaph.net - http://github.com/franckcuny
Le 11/02/2011 15:07, franck a écrit :
>> I create small patch to enhance static files serving functionality. >> During development process its important to refresh pages quickly. >> Conditional HTTP requests is the good choise. >> Seems like working, but needs more testing :) > > I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware. >
This is clearly a Plack solution. If you really need, you can maybe create a plugin for this, as soon as we add a new hook.
I would indeed not be opposed to a plugin, but I fear we open a pandora box: reimplementing any Plack middleware as Dancer plugin (which is clearly something we don't want to do). Anyways, I don't think this should go in the core of Dancer. Dancer should remain as lightweight as possible, it's code should be very specific to what it does: dispatching request to the appropriate route handler. I hope you understand my point of view ;) Regards, -- Alexis Sukrieh
On Feb 11, 2011, at 5:10 PM, Alexis Sukrieh wrote:
Le 11/02/2011 15:07, franck a écrit :
>> I create small patch to enhance static files serving functionality. >> During development process its important to refresh pages quickly. >> Conditional HTTP requests is the good choise. >> Seems like working, but needs more testing :) > > I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware. >
This is clearly a Plack solution. If you really need, you can maybe create a plugin for this, as soon as we add a new hook.
I would indeed not be opposed to a plugin, but I fear we open a pandora box: reimplementing any Plack middleware as Dancer plugin (which is clearly something we don't want to do).
Anyways, I don't think this should go in the core of Dancer. Dancer should remain as lightweight as possible, it's code should be very specific to what it does: dispatching request to the appropriate route handler.
I hope you understand my point of view ;)
I understand you clearly. But few lines of code (with no extra dependencies) which make developers' lives easier couldn't break lightweight of framework. It's my opinion only, sorry. For future i will have to patch new versions of Dancer myself :)
Regards,
-- Alexis Sukrieh
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
Le 11/02/2011 15:33, Oleg A. Mamontov a écrit :
I hope you understand my point of view ;)
I understand you clearly. But few lines of code (with no extra dependencies) which make developers' lives easier couldn't break lightweight of framework. It's my opinion only, sorry. For future i will have to patch new versions of Dancer myself :)
I understand your motivations, but more code means more risks of bugs, and as Dancer is growing very fast, our job (as core-team members) is to make sure any code addition is made with a good reason. To be frank, we're more in the mood of removing code rather than adding some ;) I think implementing that as a plugin is a good compromise: you'll get your feature without Plack and you'll be able to upgrade Dancer smoothly. We just need to implement the right hook for your plugin to be possible. -- Alexis Sukrieh
On 11/02/2011 14:04, Oleg A. Mamontov wrote:
On Feb 11, 2011, at 4:54 PM, Alexis Sukrieh wrote:
Le 10/02/2011 18:36, Oleg A. Mamontov a écrit :
Hello.
I create small patch to enhance static files serving functionality. During development process its important to refresh pages quickly. Conditional HTTP requests is the good choise. Seems like working, but needs more testing :)
I'm not sure we should do that in Dancer's core. This can be very easily achieved with a Plack Middleware.
Any specific reason not to use the appropriate middleware?
Nobody use `./bin/app.pl` for development (with no Plack) ?
I use!
On Fri, Feb 11, 2011 at 3:04 PM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
Any specific reason not to use the appropriate middleware?
Nobody use `./bin/app.pl` for development (with no Plack) ?
Out of curiosity and completely neutral with respect to the topic, is: ./bin/app.pl so much different from: plackup bin/app.pl in your experience? I was inclined not use use plackup in the first place, but after some time I understood that I had no good reason. On the other hand, it seems that sometimes there are good reasons to use it, like in this case.
From what I understand in stuff I read around, you're more or less forced to go with Plack when you're in production, so why striving to avoid it?
Cheers, Flavio.
On Feb 12, 2011, at 12:27 AM, Flavio Poletti wrote:
On Fri, Feb 11, 2011 at 3:04 PM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
Any specific reason not to use the appropriate middleware?
Nobody use `./bin/app.pl` for development (with no Plack) ?
Out of curiosity and completely neutral with respect to the topic, is:
./bin/app.pl
so much different from:
plackup bin/app.pl
in your experience? I was inclined not use use plackup in the first place, but after some time I understood that I had no good reason. On the other hand, it seems that sometimes there are good reasons to use it, like in this case.
From what I understand in stuff I read around, you're more or less forced to go with Plack when you're in production, so why striving to avoid it?
You are completely right and i agree with you. My point of view is very simple. We can delegate serve static files to Plack or anything other. No problem, but it would be good to document this recommendation in manual and remove this code from Dancer core. But if Dancer offers its own built-in functionality, it seems to me that we need to do it with maximum quality. Isn't it? Poorly made functionality worse than missing functionality because it creates ambiguity. BTW, see source code for Catalyst::Plugin::Static::Simple in _serve_static method. There is no conditional get but there is: $c->res->headers->last_modified( $stat->mtime ); What do you think, why is it done? Without this header many browsers will re-request all your static files when you navigate on your site with mouse clicks (no force page refresh). Just one simple header (Last-Modified) and you skip a lot of requests! I did not expect that such simple and obvious things can generate as much debates :(
Cheers,
Flavio.
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
On Fri, Feb 11, 2011 at 10:57 PM, Oleg A. Mamontov <oleg@mamontov.net>wrote:
We can delegate serve static files to Plack or anything other. No problem, but it would be good to document this recommendation in manual and remove this code from Dancer core. But if Dancer offers its own built-in functionality, it seems to me that we need to do it with maximum quality. Isn't it? Poorly made functionality worse than missing functionality because it creates ambiguity.
This strategy seems a bit too aggressive. I don't think the functionality is poorly made, it just is a basic one and lacks some more advanced feature. If you want the advanced features, you can hop to more sophisticated solutions. This is how I see it. I can agree in documenting it, but keep in mind that Dancer is a young project!
BTW, see source code for Catalyst::Plugin::Static::Simple in _serve_static method. There is no conditional get but there is: $c->res->headers->last_modified( $stat->mtime ); What do you think, why is it done? Without this header many browsers will re-request all your static files when you navigate on your site with mouse clicks (no force page refresh). Just one simple header (Last-Modified) and you skip a lot of requests!
So maybe you agree with Alexis here: I don't know anything about Catalyst and its architecture, but the functionality you're talking about is in a "::Plugin::" module, which seems to point in the same direction.
I did not expect that such simple and obvious things can generate as much debates :(
I see the debate as a winning point, because it is productive and not destructive. So far, I see the potential for a Plack middleware, a change to the API to add some hook and a related plugin, and some more documentation to make it all clearer. Just try to consider this thing a bit less obvious ;-) Cheers, Flavio.
On Feb 12, 2011, at 8:00 AM, Flavio Poletti wrote:
On Fri, Feb 11, 2011 at 10:57 PM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
We can delegate serve static files to Plack or anything other. No problem, but it would be good to document this recommendation in manual and remove this code from Dancer core. But if Dancer offers its own built-in functionality, it seems to me that we need to do it with maximum quality. Isn't it? Poorly made functionality worse than missing functionality because it creates ambiguity.
This strategy seems a bit too aggressive. I don't think the functionality is poorly made, it just is a basic one and lacks some more advanced feature. If you want the advanced features, you can hop to more sophisticated solutions. This is how I see it.
I can agree in documenting it, but keep in mind that Dancer is a young project!
BTW, see source code for Catalyst::Plugin::Static::Simple in _serve_static method. There is no conditional get but there is: $c->res->headers->last_modified( $stat->mtime ); What do you think, why is it done? Without this header many browsers will re-request all your static files when you navigate on your site with mouse clicks (no force page refresh). Just one simple header (Last-Modified) and you skip a lot of requests!
So maybe you agree with Alexis here: I don't know anything about Catalyst and its architecture, but the functionality you're talking about is in a "::Plugin::" module, which seems to point in the same direction.
Key moment: Catalyst::Plugin::Static::Simple is core module of Catalyst distribution.
I did not expect that such simple and obvious things can generate as much debates :(
I see the debate as a winning point, because it is productive and not destructive. So far, I see the potential for a Plack middleware, a change to the API to add some hook and a related plugin, and some more documentation to make it all clearer. Just try to consider this thing a bit less obvious ;-)
Cheers,
Flavio. _______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
On Sat, Feb 12, 2011 at 8:36 AM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
On Feb 12, 2011, at 8:00 AM, Flavio Poletti wrote:
BTW, see source code for Catalyst::Plugin::Static::Simple in _serve_static method. There is no conditional get but there is: $c->res->headers->last_modified( $stat->mtime ); What do you think, why is it done? Without this header many browsers will re-request all your static files when you navigate on your site with mouse clicks (no force page refresh). Just one simple header (Last-Modified) and you skip a lot of requests!
So maybe you agree with Alexis here: I don't know anything about Catalyst and its architecture, but the functionality you're talking about is in a "::Plugin::" module, which seems to point in the same direction.
Key moment: Catalyst::Plugin::Static::Simple is core module of Catalyst distribution.
As I was said, I'm really talking about something I don't know at all, but naming this module "::Plugin::" seems to imply that you load it only if you need it, and you don't get this functionality as a "core" functionality without loading it. If this applies, are we really talking about having this plugin in the core Dancer or as a separate distribution? If this does not apply, are we talking about cloning Catalyst? Seeing it from another perspective, the "static file serving" functionality in Dancer might be basic but covers the needs of many and does it in a non-buggy and consistent way, although suboptimal. IMHO, Dancer is more and should be more concentrated on the dynamic part, so this approach seems perfectly reasonable. I have probably drifted on the unconstructive side of the discussion, anyway: although I might agree with you with the patch, I think that the core Dancer development group and its "Architect" deserve the right to say no, especially if they have a reason that is inspired to the principles that steer their development. Cheers, Flavio.
Hi Flavio, Oleg, On 12/02/2011 10:00, Flavio Poletti wrote:
I have probably drifted on the unconstructive side of the discussion, anyway: although I might agree with you with the patch, I think that the core Dancer development group and its "Architect" deserve the right to say no, especially if they have a reason that is inspired to the principles that steer their development.
I'd like to underline some remarks regarding this discussion: * I'm not opposed to provide a simple way for supporting a Last-Modified header * Although, the core of Dancer should be kept as minimal as possible, we try very hard to resist the temptation to add stuff in it; because this is the only way we can provide a mature and stable project eventually. * More code in the core means more potential bugs, more backward compatibility issues and more time needed by the core-team for maintaining the projet (and refactoring becomes harder when you have more code to care about). Given these reasons, I'd vote for a plugin (let's say Dancer::Plugin::LastModified) which would be shipped with the core distribution. Just like Dancer::Plugin::Ajax is. Being shipped with the core distribution doesn't mean being core. Do you see what I mean? By isolating these extra code lines in a plugin we protect ourselves for future issues regarding that part of the code. My concerns are mainly maintenance ones, I've nerver doubted the benefit of having this feature (altough a Plack middleware exists and we should keep it very simple). So, do we have an agreement? -- Alexis Sukrieh
On 12 February 2011 13:01, Alexis Sukrieh <sukria@sukria.net> wrote:
Hi Flavio, Oleg,
I'd like to underline some remarks regarding this discussion:
[...]
By isolating these extra code lines in a plugin we protect ourselves for future issues regarding that part of the code.
My concerns are mainly maintenance ones, I've nerver doubted the benefit of having this feature (altough a Plack middleware exists and we should keep it very simple).
So, do we have an agreement?
Having followed the discussion but not said anything as of yet, I'd like to say that I fully agree with what Alexis just said. It looks to me as a good compromise, and an adequate solution for users in need for this feature.
On Feb 15, 2011, at 7:49 PM, damien krotkine wrote:
On 12 February 2011 13:01, Alexis Sukrieh <sukria@sukria.net> wrote:
Hi Flavio, Oleg,
I'd like to underline some remarks regarding this discussion:
[...]
By isolating these extra code lines in a plugin we protect ourselves for future issues regarding that part of the code.
My concerns are mainly maintenance ones, I've nerver doubted the benefit of having this feature (altough a Plack middleware exists and we should keep it very simple).
So, do we have an agreement?
Having followed the discussion but not said anything as of yet, I'd like to say that I fully agree with what Alexis just said. It looks to me as a good compromise, and an adequate solution for users in need for this feature.
This feature is one of basic principles in hyper text transfer protocol :) Perhaps creating something like 'Dancer::Plugin::Static' (implemented static files serving functionality completely) is more adequate way?
_______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov mailto: oleg@mamontov.net jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
On Tue, Feb 15, 2011 at 6:33 PM, Oleg A. Mamontov <oleg@mamontov.net> wrote:
On Feb 15, 2011, at 7:49 PM, damien krotkine wrote:
On 12 February 2011 13:01, Alexis Sukrieh <sukria@sukria.net> wrote:
Hi Flavio, Oleg,
I'd like to underline some remarks regarding this discussion:
[...]
By isolating these extra code lines in a plugin we protect ourselves for future issues regarding that part of the code.
My concerns are mainly maintenance ones, I've nerver doubted the benefit of having this feature (altough a Plack middleware exists and we should keep it very simple).
So, do we have an agreement?
Having followed the discussion but not said anything as of yet, I'd like to say that I fully agree with what Alexis just said. It looks to me as a good compromise, and an adequate solution for users in need for this feature.
This feature is one of basic principles in hyper text transfer protocol :)
Perhaps creating something like 'Dancer::Plugin::Static' (implemented static files serving functionality completely) is more adequate way?
I like this idea
_______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- Cheers, Oleg A. Mamontov
mailto: oleg@mamontov.net
jabber: lonerr@charla.mamontov.net icq uin: 79-521-617 cell: +7-903-798-1352
_______________________________________________ Dancer-users mailing list Dancer-users@perldancer.org http://www.backup-manager.org/cgi-bin/listinfo/dancer-users
-- franck cuny http://lumberjaph.net - http://github.com/franckcuny
On Sat, Feb 12, 2011 at 1:01 PM, Alexis Sukrieh <sukria@sukria.net> wrote:
Hi Flavio, Oleg, [...] Being shipped with the core distribution doesn't mean being core. Do you see what I mean?
You found the right words.
By isolating these extra code lines in a plugin we protect ourselves for future issues regarding that part of the code.
My concerns are mainly maintenance ones, I've nerver doubted the benefit of having this feature (altough a Plack middleware exists and we should keep it very simple).
So, do we have an agreement?
I'm not in the position to express anything here, apart from opinions. I was only trying to give a perspective on why the functionality had been partially refused and shifted to something "external" - plugin or middleware. As long as you are happy I think that we have an agreement! Cheers, Flavio.
participants (8)
-
Alberto Simoes -
Alexis Sukrieh -
damien krotkine -
David Precious -
Flavio Poletti -
franck -
Oleg A. Mamontov -
Richard Huxton