Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wildcard glob support #178

Open
ghost opened this issue Mar 5, 2021 · 10 comments · May be fixed by #375
Open

Wildcard glob support #178

ghost opened this issue Mar 5, 2021 · 10 comments · May be fixed by #375
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@ghost
Copy link

ghost commented Mar 5, 2021

It looks like passing a glob to wildcard was a breaking change in the 4.x release (#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 #172 was added, but wasn't sure if that would actually allow the same behavior or if there were differences.

@mcollina
Copy link
Member

mcollina commented Mar 5, 2021

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.

@mcollina mcollina added semver-minor Issue or PR that should land as semver minor good first issue Good for newcomers labels Mar 5, 2021
@ghost
Copy link
Author

ghost commented Mar 5, 2021

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
Copy link
Member

mcollina commented Mar 8, 2021

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

@mcollina
Copy link
Member

mcollina commented Mar 8, 2021

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
Copy link
Contributor

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
Copy link

yaneony commented Apr 18, 2023

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

@mcollina
Copy link
Member

Calling it globPattern would be good.

@yaneony
Copy link

yaneony commented Apr 19, 2023

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

@mcollina
Copy link
Member

That would also be ok

@yaneony yaneony linked a pull request Apr 19, 2023 that will close this issue
4 tasks
@Fdawgs Fdawgs linked a pull request Apr 19, 2023 that will close this issue
4 tasks
@yaneony
Copy link

yaneony commented Apr 19, 2023

Need help: #375 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants