fastify / fastify-static

Plugin for serving static files as fast as possible
MIT License
433 stars 97 forks source link

Wildcard glob support #178

Open ghost opened 3 years ago

ghost commented 3 years ago

It looks like passing a glob to wildcard was a breaking change in the 4.x release (https://github.com/fastify/fastify-static/pull/175), but it's unclear why it was removed. It's an extremely valuable feature and something I've used across multiple applications. Is it possible to revert that change and keep the functionality, or is there a workaround to keep similar behavior without having to rework directory structures?

For one of the use cases specifically, by using globs, it's possible to keep static assets next to the relevant code they're used with rather than having to maintain a separate assets directory. It's in a similar vein of keeping your tests next to the code versus having a separate tests directory, etc.

It'd be awesome to keep this functionality if there wasn't a strong reason to remove it.

EDIT: I did see that https://github.com/fastify/fastify-static/pull/172 was added, but wasn't sure if that would actually allow the same behavior or if there were differences.

mcollina commented 3 years ago

The problem with the wildcard: 'globPattern' option was that it disabled some features, most notably adding it would fail to detect new files.

If somebody would like to work on this problem, adding another option in to specify the glob pattern would be good: it should work with wildcard both true and false.

ghost commented 3 years ago

The problem with the wildcard: 'globPattern' option was that it disabled some features, most notably adding it would fail to detect new files.

I thought that was a feature, not a bug :P

To clarify, I'm not saying it needs to be added back if there is a different way to do it. It appears the allowedPaths option could be a viable alternative, but I didn't know if there were drawbacks to using that versus the original wildcard option.

E.g.

- wildcard: '*.{css,png,woff2}',
+ allowedPath: (path) => /\.(?:css|png|woff2)$/.test(path),

Could you confirm if the two lines are effectively the same? In testing, it appears to work as expected, but didn't want to get hit with a gotcha later because I misunderstood something.

On a side note, allowedPaths struck me as an odd name (it's actually why I didn't notice it when I first read through everything). It only dawned on me when I noticed the PR / old linked issue of mine that talked about filtering. I can create a separate issue if you'd be open to it, but is it possible to rename allowedPaths to filter, or possibly make filter an alias?

Appreciate the help :)

mcollina commented 3 years ago

Calling it filter() would actually be way better! Would you mind to send a PR adding it as an alias for allowedPath?

mcollina commented 3 years ago

The problem with the wildcard: 'globPattern' option was that it disabled some features, most notably adding it would fail to detect new files.

I thought that was a feature, not a bug :P

Well it was but we got a few issue reports that "it failed to detect new files" and it was really confusing for a few users. So, we removed it. It seems it's easy to add it back via filter or similar if you or somebody else is willing to put the effort to work on it.

AndersCan commented 2 years ago

I was the one who originally added the wildcard: globPattern feature and never thought it would be so confusing 😅. I fully understand why it was removed though.

I have now upgraded to 4.x and use something like this:

  const prefix = '/public';
  const assetRoot = path.join(process.cwd(), '/assets');
  app.register(fastifyStatic, { root: assetRoot, wildcard: true, decorateReply: true });
  const assetPaths = glob.sync(path.join(assetRoot, '/**/*.{css,js}'));

  const assets = assetPaths.map((path) => {
    return {
      diskPath: path.replace(assetRoot, ''),
      routePath: path.replace(assetRoot, prefix),
    };
  });

  for (const asset of assets) {
    app.get(asset.routePath, (_request, reply) => {
      reply.sendFile(asset.diskPath, assetRoot);
    });
  }

I think this should work similar to before, but maybe one of the maintainers can confirm.

I was considering using allowedPath as mentioned above, but I was worried about the performance cost and potentially opening the server to a Regular expression Denial of Service.

yaneony commented 1 year ago

Would it be okay to add something like globOverride for people who want make own behavior?

mcollina commented 1 year ago

Calling it globPattern would be good.

yaneony commented 1 year ago

Wouldn't it be better to allow to pass additional(all) options to glob too?!

mcollina commented 1 year ago

That would also be ok

yaneony commented 1 year ago

Need help: https://github.com/fastify/fastify-static/pull/375#issuecomment-1514768133