jfortunato / esbuild-plugin-manifest

Esbuild plugin for building an asset manifest file.
MIT License
31 stars 5 forks source link

feat: New manifest format inspired by Vite #25

Open marcospereira opened 5 months ago

marcospereira commented 5 months ago

References

Fixes #11.

Depends on #24.

What

This is inspired by Vite's manifest used for backend integrations:

https://vitejs.dev/guide/backend-integration

It adds a few extra pieces of information to improve HTTP cache handling. For example, it will be possible to set the ETag header:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag

And finally, a hash to allow the usage of Subresource integrity:

https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

Warning

This is a breaking change since the generated format is incompatible with the current version.

jfortunato commented 5 months ago

Hey @marcospereira thanks for the PR! I've not had time to review everything yet but I have a couple immediate thoughts/questions..

1) Are the dependency/build updates actually necessary for the feature being implemented?

2) I'm wondering if this might be better implemented as a generic map function with a user supplied callback. What I mean is that the proposed changes here kind of lock us in to supporting hardcoded etag/subresource extensions even when a user may not utilize them, or they may utilize different implementations (sha256? sha512?) So I'm suggesting some kind of map function that would allow the user to implement their own functions.

This way in the simplest and most common case, the manifest file remains a basic string => string mapping, but we can allow an advanced user to hook into the process and generate a string => any or something, where the any could be like your structure or whatever the user wants really, but without fields they don't need:

{
  "file": "example-4QTUNIID.js",
  "integrity": "<hash>"
}
marcospereira commented 5 months ago

Hey @jfortunato, thanks for the feedback. Addressing your questions:

1. Are the dependency/build updates actually necessary for the feature being implemented?

CI currently runs with Node versions that aren't supported anymore. To run with Node >=18, I had to update the dependencies. There is https://github.com/jfortunato/esbuild-plugin-manifest/pull/24 as a separate pull request to isolate those changes.

2. I'm wondering if this might be better implemented as a generic map function with a user supplied callback. What I mean is that the proposed changes here kind of lock us in to supporting hardcoded etag/subresource extensions even when a user may not utilize them, or they may utilize different implementations (sha256? sha512?) So I'm suggesting some kind of map function that would allow the user to implement their own functions.

I also considered having a map function or allowing users to configure the algo used to generate the subresource integrity hash. But, overall, I think these are good defaults, even if they aren't useful for every use case. For my use case, I would still use a map function to add extra information (about gzip/brotli encodings + webp/avif images) that does not belong here. I do like the idea that one only needs to add the plugin and get data that helps to follow best practices.

That said, I also agree that balancing what you get out of the box may be tricky, and I am open to removing whichever you think shouldn't be part of the plugin.

jfortunato commented 5 months ago

A generic callback will allow us to keep the plugin simple but at the same time enable many advanced use cases, so I think we should go that way. This seems to be the approach webpack-manifest-plugin (see #35) has taken as well. We can also update the docs with a specific example that demonstrates how it can be used for SRI.

marcospereira commented 5 months ago

A generic callback will allow us to keep the plugin simple but at the same time enable many advanced use cases, so I think we should go that way.

I agree with having a callback to customize the manifest further. But this does not exclude having a richer manifest out-of-the-box.

This seems to be the approach webpack-manifest-plugin (see #35) has taken as well. We can also update the docs with a specific example that demonstrates how it can be used for SRI.

That decision was made in 2017. SRI was relatively new at the time and possibly not even widely supported by browsers. I wouldn't use a decision made ~7 years ago as the baseline for today, especially in an ecosystem as dynamic as the JS one.

But honestly, I don't want to be pushy here, and I fully understand your perspective on keeping the plugin as simple as possible. I'm cool with closing this or turning it into a documentation PR if that is what you decide as the path forward.