Extended dir-list information #241

Jelenkee · 2021-10-04T17:01:56Z

I added some features to the dirlist.
Additionally to the names I added the stats of the dirs/files and some extended information for the dirs.
You can change the format (html/json) via the URL-Parameter.

Furthermore I replaced the callbacks with promises.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

-added stats and more info to dir list

mcollina · 2021-10-04T17:20:02Z

index.js

@@ -164,7 +165,7 @@ async function fastifyStatic (fastify, opts) {
      if (err.code === 'ENOENT') {
        // if file exists, send real file, otherwise send dir list if name match
        if (opts.list && dirList.handle(pathname, opts.list)) {
-          return dirList.send({ reply, dir: dirList.path(opts.root, pathname), options: opts.list, route: pathname })
+          return dirList.send({ request, reply, dir: dirList.path(opts.root, pathname), options: opts.list, route: pathname })


There is no need to add request, it's available on reply.request.

mcollina · 2021-10-04T17:24:31Z

lib/dirList.js

+
+      async function walk (dir) {
+        const files = await fs.readdir(dir)
+        await Promise.all(files


This is going to become problematic on very large folders. You should use p-map instead.

What is a very large folder?
I tested it with my home folder (91593 files, 19091 folders)
Promise.all: ~1.7s
p-map: ~1.2s

So there is an improvement

-used p-map

mcollina

I'm sorry about the back-and-forth, the files.map() throw me off-guard and made me assume we needed to use p-map. We can just use p-limit as there is no need to "map" things.

mcollina · 2021-10-05T12:34:14Z

lib/dirList.js

+      return entries
+    }
+
+    await Promise.all(files.map(async filename => {


please use p-limit here. We are not really interested in the result of all the promises.

mcollina · 2021-10-05T12:35:13Z

lib/dirList.js

+
+      async function walk (dir) {
+        const files = await fs.readdir(dir)
+        await pMap(files, async filename => {


If we are not interested in the result, maybe we could just use p-limit instead.

mcollina

lgtm, thx

climba03003

Will it has any Unhandled Exception when we change the interface from callback to async-await?

fastify-static/index.js

Lines 145 to 150 in 8da6140

    
           return dirList.send({ 
        
             reply, 
        
             dir: path, 
        
             options: opts.list, 
        
             route: pathname 
        
           })

fastify-static/index.js

Line 164 in 8da6140

    
           return dirList.send({ reply, dir: dirList.path(opts.root, pathname), options: opts.list, route: pathname })

Fixes are needed

mcollina · 2021-10-06T10:59:23Z

@Jelenkee could you add a .catch((err) => { reply.send(err) }) in those places? Also avoid to return that promise to the caller.

Jelenkee · 2021-10-06T15:57:06Z

Mmh... When I add the catch method to the calling method, the coverage for the tests is below 100% 😆
And I cannot add a test for this because the only error that can occur is already catched and replied with 404 to the client

climba03003 · 2021-10-06T16:03:45Z

Mmh... When I add the catch method to the calling method, the coverage for the tests is below 100% 😆 And I cannot add a test for this because the only error that can occur is already catched and replied with 404 to the client

If we cannot think of a case that will throw for a promise currently. Using t.mock for dirList.send is acceptable to simulate that error.

It is always good to handle all promise properly (e.g. .catch or .finally), even we cannot think a error at this stage.

Remember do not return a promise inside a stream event callback.

climba03003

LGTM.

mcollina

lgtm

Jelenkee added 5 commits October 3, 2021 18:51

-refactored dirlist to use promises

49eff6a

-added stats and more info to dir list

-added lastModified

7112d1e

-added format url parameter

8ee1641

-added docs

8b2b95e

-fixed compatibility with node 10

98503ab

mcollina reviewed Oct 4, 2021

View reviewed changes

-removed unnecessary request

353ca84

-used p-map

mcollina reviewed Oct 5, 2021

View reviewed changes

-replaced p-map with p-limit

6d1094b

mcollina previously approved these changes Oct 5, 2021

View reviewed changes

mcollina requested review from climba03003 and Eomm October 5, 2021 16:22

-removed logging

3769c3c

climba03003 reviewed Oct 5, 2021

View reviewed changes

Jelenkee added 2 commits October 6, 2021 19:43

-added error catch

091b356

-improved test

4acf69e

climba03003 approved these changes Oct 6, 2021

View reviewed changes

mcollina approved these changes Oct 7, 2021

View reviewed changes

mcollina merged commit 9e3286c into fastify:master Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended dir-list information #241

Extended dir-list information #241

Jelenkee commented Oct 4, 2021 •

edited

Loading

mcollina Oct 4, 2021

Jelenkee Oct 4, 2021

mcollina Oct 4, 2021

Jelenkee Oct 4, 2021

mcollina left a comment

mcollina Oct 5, 2021

Jelenkee Oct 5, 2021

mcollina Oct 5, 2021

Jelenkee Oct 5, 2021

mcollina left a comment

climba03003 left a comment •

edited

Loading

mcollina commented Oct 6, 2021

Jelenkee commented Oct 6, 2021

climba03003 commented Oct 6, 2021 •

edited

Loading

climba03003 left a comment

mcollina left a comment

	return dirList.send({
	reply,
	dir: path,
	options: opts.list,
	route: pathname
	})

Extended dir-list information #241

Extended dir-list information #241

Conversation

Jelenkee commented Oct 4, 2021 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

climba03003 left a comment • edited Loading

Choose a reason for hiding this comment

mcollina commented Oct 6, 2021

Jelenkee commented Oct 6, 2021

climba03003 commented Oct 6, 2021 • edited Loading

climba03003 left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Jelenkee commented Oct 4, 2021 •

edited

Loading

climba03003 left a comment •

edited

Loading

climba03003 commented Oct 6, 2021 •

edited

Loading