dart-lang / shelf

Web server middleware for Dart
https://pub.dev/packages/shelf
BSD 3-Clause "New" or "Revised" License
893 stars 125 forks source link

Add context to `Response` for including `file` and `file_not_found` to be identified by other `shelf` `Middleware`. #395

Closed gmpassos closed 1 month ago

gmpassos commented 7 months ago

Updated:

With the Response.context populated with the processed File, other Middleware can manipulate the Response in an optimized way and also ensure the correct processed File. Without that, another Middleware would need to guess the processed File, and in some cases, re-resolve File.stat and also fix the directory path to the "index.html" path.

kevmoo commented 7 months ago

i see where you're going here.

I'd want to chat with @natebosch or @devoncarew on their thoughts

This change should be documented and tested, too!

gmpassos commented 7 months ago

With the Response.context populated with the processed File, other Middleware can manipulate the Response in an optimized way and also ensure the correct processed File. Without that, another Middleware would need to guess the processed File, and in some cases, re-resolve File.stat and also fix the directory path to the "index.html" path.

gmpassos commented 7 months ago

In my use case, I maintain an in-memory cache of shelf Responses. Then I need to verify if the cached Response File has changed using File.stat.

However, my Middleware currently has to recalculate the Response File before caching the Response. This leads to a doubling of the initial Response time due to the additional File resolution, involving calls to .existsSync and statSync, as well as the resolution of the Directory to index.html.

Keep in mind that the primary purpose of a cache is to enhance response time, and the negative impact of these extra calls to statSync is significant.

gmpassos commented 6 months ago

What's the next step? Regards.

kevmoo commented 6 months ago

need changelog entry and tests

natebosch commented 3 months ago

Is this still something you're interested in landing - can you take a look at adding tests?

gmpassos commented 3 months ago

This is important.

I will try to add some tests, but I don't have time for that in the next 15 days.

natebosch commented 3 months ago

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

Also cc @brianquinlan - can you think of any risks in keeping the File object in the heap for longer than we would have previously.

gmpassos commented 3 months ago

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

You can take a look at real-world code that is waiting for this: https://github.com/Colossus-Services/bones_api/blob/master/lib/src/bones_api_server.dart#L1788

Note that if 'shelf_static:file' and 'shelf_static:file_not_found' are not present, we need to recalculate the File path, validate it, and ensure (again) that it's inside the "root" directory (simulating the same shelf:createStaticHandler logic).

The context['file'] is then used by the cache "middleware", which will use the File instance to validate the cached files and dispose of the cached data if the File changes: https://github.com/Colossus-Services/bones_api/blob/master/lib/src/bones_api_server_cache.dart#L351

brianquinlan commented 3 months ago

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

Also cc @brianquinlan - can you think of any risks in keeping the File object in the heap for longer than we would have previously.

A File object doesn't represent any native resources so keeping it alive shouldn't do anything except cost a bit of memory.

gmpassos commented 1 month ago

@natebosch @brianquinlan Next step?

gmpassos commented 1 month ago

This PR will continue at PR https://github.com/dart-lang/shelf/pull/436