feat(rollup-plugin-import-meta-assets): init plugin

[hsablonniere]

Let's try to discuss about this plugin 😉

• Features/options
• Implementation
• Tests
• Naming

Let me know what you think about this...

[changeset-bot]

💥 No Changeset

Latest commit: c997b17

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[LarsDenBakker]

need to check if this works on node v10

[hsablonniere]

Will do. Official docs says v10. Local test with latest v10 works.

[hsablonniere]

@LarsDenBakker I fixed code and docs according to your review in an auto fixup commit so it's easier for you to see what I changed and we'll be able to rebase before merging.

[daKmoR]

If I want to optimize my SVG, should it be a transform option of this plugin or can this be done properly outside of this?

I think doing it outside of this plugin would be more powerful - especially as there might be other plugins that add assets as well. For example, we surely want to also support optimizations for files references in html like <link rel="stylesheet" href="styles.css">... but that will be in @web/rollup-plugin-html and it would feel strange if both then offer a way to transform css/svg/...

imho there should be an official rollup hook to transform assets (iirc currently there is none 🙈)

ideally, you would at some point install rollup-plugin-optimize-svg and it would optimize all svg assets no matter which plugin it adds.

[hsablonniere]

@daKmoR Hey 😉

• Where are you quoting this from?
• Are you saying we should not ship it with this option?

[LarsDenBakker]

I filed a feature request on rollup for this: rollup/rollup#3778

In the meantime we have to do something. If we don't include a transform option, we still need to make this possible for users in some way. We either need to make a separate plugin for this, or we need to document for people how to do this manually:

// untested code
export default {
  plugins: [
    {
      generateBundle(options, bundle) {
        for (const name of Object.keys(bundle)) {
          if (name.endsWIth('.json') {
            bundle[key] = JSON.stringify(JSON.parse(bundle[key].content));
          }
        }
      }
    }
  ]
}

@daKmoR what do you think?

[daKmoR]

• Where are you quoting this from?

from you 😬

https://github.com/modernweb-dev/web/blob/983ba209dbe8222eb3669ba5ce8e0d198c2add10/packages/rollup-plugin-import-meta-assets/EXPLAINER.md#questions--ideas

• Are you saying we should not ship it with this option?

jup

@daKmoR what do you think?

the rollup transformFile hook looks awesome 👍
and having an example in the readme on how to do via generateBundle is good enough until transformFile ships in rollup I would say 🤗

[hsablonniere]

@daKmoR 😆 you're right, this explainer should not be part of this PR. It will be integrated in a reworked form in the guides. Sorry.

About the transformFile hook, I like the idea. I'll ask my questions there...

[hsablonniere]

@LarsDenBakker I tried your example with generateBundle in my unit test:

{
  generateBundle (options, bundle) {
    for (const name of Object.keys(bundle)) {
      if (name.endsWith('.svg')) {
        bundle[name].source = bundle[name].source + '<!-- minified -->\n';
      }
    }
  },
},

It works for the content but the file hash is not updated.

If rollup supported such a transformFile hook, it would maybe support rewriting import statements to support hash changes but I guess it would not rewrite new URL('path/to/asset', import.meta.url). If it did, this plugin would be useless.

[LarsDenBakker]

Good catch... we might need to use another hook like buildEnd. Not sure if they expose the information there.

[hsablonniere]

I don't see how we can make this work. I mean in buildEnd, you would have to recompute all assets hashes, update all ES modules referencing assets which means again, updating hashes of ES modules.

[hsablonniere]

@LarsDenBakker On another note, I'm using "snapshots" files to compare with the output of some of my tests. Seems like I need a .prettierignore. Is it ok if I add one?

[LarsDenBakker]

Yes, go ahead.

[daKmoR]

so transformFile will need to be executed before calculating the file hash? does rollup already have a hook for that?

hashing the filename and renaming it... should imho be part of a plugin and not part of rollup as it's probably out of scope (and we wanna keep the core small and performant I would say)

the hashing + rename we already have in this plugin 👍

So overall I think this structure would cover multiple "use cases"

• only copy @web/plugin-rollup-copy
• only hashing for js import meta @web/rollup-plugin-import-meta-assets
• only hashing for html <link href="...">, <img src="...">, ... @web/rollup-plugin-html (or a separate plugin?)
• only optimize svgs @web/rollup-plugin-optimize-svg

Combine all of those

Want copy + svg optimization? @web/plugin-rollup-copy + @web/rollup-plugin-optimize-svg

Want hash + svg optimization? @web/rollup-plugin-import-meta-assets + @web/rollup-plugin-html + @web/rollup-plugin-optimize-svg

Name

Writing it out like this I think a better name could be

• @web/rollup-plugin-hash-import-meta-files
• @web/rollup-plugin-import-meta-files
• @web/rollup-plugin-hash-dynamic-import-files

Follow-ups

Having plugins to optimize svg, json, css, images? would be awesome

[hsablonniere]

@LarsDenBakker

• I fixed the prettier problems I had
• I have no idea why the Browserstack and Sauce Labs are failing, do you have any idea?
• I'm not sure but it seems like the windows version is failing because of EOL differences between expected and actual 😢

[hsablonniere]

@daKmoR

I see the big picture you're drawing; I mostly agree with the set of decoupled specialized plugins you're proposing; but I don't see how this can be done properly without non trivial updates to Rollup.

I don't have an advanced understanding of Rollup's inner behaviour so please correct me if I'm wrong. From reading the docs and the code, it seems like (right now) Rollup is tailored for ES modules but not for really for assets. Almost all asset plugins get away with it by importing assets as ES modules and producing ES modules exporting the asset as a string.

I don't see how the features required to achieve this multiple plugins setup you're describing could land before a few months (or more). Therefore, I think we should ship this plugin as is, with the transform option. This option is very similare to the transform option of this copy plugin. In its current form, this plugin solves problems. It does not solves them in the smartest/cleanest way but it does.

Now let me try to comment on your messages 😉

so transformFile will need to be executed before calculating the file hash? does rollup already have a hook for that?

Well, Rollup already has a transform hook but as I said, it's only for modules. I think we need transformAsset.

Rollup already knows what an asset is and has a dedicated config for how to rename and maybe hash them : output.assetFileNames. If you use import.meta.ROLLUP_FILE_URL_[ref] like I did in this plugin, it will manage the asset file name (relative, hash...) for you inside your modules source.

hashing the filename and renaming it... should imho be part of a plugin and not part of rollup as it's probably out of scope (and we wanna keep the core small and performant I would say)

I don't think so.

Rollup's core already handles this.
I knows how to rewrite modules' relative paths, file names and hashes.
For assets, it's very low level right now, but in a way, with import.meta.ROLLUP_FILE_URL_[ref], Rollup already knows how to handle hashing asset file names with the config I mentioned earlier.

I think Rollup should continue to handle this in core but maybe should open a bit more hooks and features related to assets.

the hashing + rename we already have in this plugin +1

This plugin has no logic related to hashing, it doesn't even know Rollup can do this.
It only knows how to rewrite relative asset references inside ES modules like this new URL('path/to/asset', import.meta.url) to import.meta.ROLLUP_FILE_URL_[ref] so Rollup can take over and do the rest.

So overall I think this structure would cover multiple "use cases"

• only copy @web/plugin-rollup-copy

I agree with this. If Rollup provides a transformAsset hook, the copy plugin should only focus on copying.

• only hashing for js import meta @web/rollup-plugin-import-meta-assets

This is where I don't understand.
This plugin is not about hashing.

It's about looking for new URL('path/to/assets', import.meta.url) expressions in ES modules and then:

• ask Rollup to produce an asset so it can take over the rest
• and rewrite the new URL() expression so the relative path still works after the bundle is created (using Rollup import.meta.ROLLUP_FILE_URL_[ref]).

I agree with you that once Rollup ships a transformAsset hook, we could remove the feature from this plugin. Again, I don't see this happening soon.

• only hashing for html <link href="...">, <img src="...">, ... @web/rollup-plugin-html (or a separate plugin?)

I don't know much about @web/rollup-plugin-html but yes, if it can detect assets from those patterns, it could in a way be the HTML (instead of ES modules) equivalent of @web/rollup-plugin-import-meta-assets.

• only optimize svgs @web/rollup-plugin-optimize-svg

If Rollup provides a transformAsset hook, I see how we could have plugins to optimize assets (SVG, JSON, CSS...).

Personal taste: for the very case of optimizing SVGs, there are so much options that I'd rather use the hook directly with svgo and my set of options (one less "pass-through dependency). For other more plug-and-play situations with less options, I could use such plugins.

That's my opinion on this but maybe it's biased because I spent some time working on it and it solves exactly the problem I have in its current form :-D

[LarsDenBakker]

You're right, rollup core does the hashing.

Looking through the docs, I don't see any other way to get the emitted assets :( At the same time, you don't necessarily need to rely on the hashing of the file content after minifying. It might actually less ideal, because a change in minifier will bust the cache.

@hsablonniere what do you think about documenting the generateBundle approach for new and see if we can get it in rollup core? It would be a shame if it gets added to rollup core pretty quickly and we have to adjust again.

[daKmoR]

@hsablonniere really good feedback and explanation - I think I understand it better now - thx 🤗

I agree that the hash of the original content should be good enough for now... so the generateBundle should be good enough for now.

is the name still open to discussion? 😬 as most of the things will be most likely called with file... e.g. emitFile, transformFile shall we call it better @web/rollup-plugin-import-meta-files?

[hsablonniere]

@LarsDenBakker @daKmoR

First point, thanks for your feedbacks, this discussion and those details are important. It feels good to have challenging opinions on this 👍

Hashing and cache busting

It might actually less ideal, because a change in minifier will bust the cache.

I agree that the hash of the original content should be good enough for now... so the generateBundle should be good enough for now.

I have a strong opinion on the fact that a change in the minifying config MUST bust the cache. I got bitten by this twice in the last 6 months. Once with terser and Leaftlet heatmap global behaviour; and once with the HTML minifier of LitElement templates. In both situation, I had to update the minifying config to fix the bug.

You could say it's rare and it's not a really a problem for assets like CSS or SVG and I would agree.

The other reason is the work I'm doing on our CDN stuffs. I intend to store all generated files with a hashed name on an S3 compatible object storage, without the version in the URL. This has many benefits but I need a strong "content is different => name is different".

Just like you, I like aiming for "good enough" but I feel like my PR contains a solution that gives the best result with a good enough API (some plugin config duplication if you're using the HTML plugin which I'm not).

It seems like you're proposing that we aim for the best API (no transform mixed with this plugin which I agree is best) with a good enough result (broken hashed names which I don't agree with).

Waiting for Rollup

@hsablonniere what do you think about documenting the generateBundle approach for new and see if we can get it in rollup core? It would be a shame if it gets added to rollup core pretty quickly and we have to adjust again.

Looking at many Rollup issue threads yesterday, I don't have the feeling such a feature would be added quickly. Even if it did, users would have to update their Rollup to benefit from this and we're not sure it would land in semver minor.

NOTE: My all "this will not happen" is a secret strategy to dare Luke and his team to ship this quickly 😉

Sorry if I sound like a broken record but I think we should ship this as is. We can start lobbying and working on a transformAsset hook in Rollup just after. While we wait, Rollup users can benefit from the new URL('', i.m.u) pattern (with transform/minification) right now, with their current version.

If such a hook lands tomorrow in core, users would have to update Rollup. That's a good opportunity for them to update their plugin configs and move away from the transform option of this plugin to a new plugin we would provide that relies on the new more generic transform hook.

Documenting the generateBundle approach

As I explained in the previous sections, I don't believe in this approach so I don't feel like writing it myself. Feel free to add it but you've been warned that I think it's not a good suggestion to advocate.

Naming

is the name still open to discussion? grimacing as most of the things will be most likely called with file... e.g. emitFile, transformFile shall we call it better @web/rollup-plugin-import-meta-files?

Of course, it's always open to discussion 😉

At this point I just want this to be shipped so I can use it in my projects so I don't care so much about the name but if you want my advice:

This very plugin deals with assets. It calls emitFile() with type: "asset".
The other type is "chunk" and I don't see why we would ever need to emit a chunk with the new URL('', i.m.u) pattern.

See https://rollupjs.org/guide/en/#thisemitfileemittedfile-emittedchunk--emittedasset--string

Rollup already has a transform hook for modules: https://rollupjs.org/guide/en/#transform and a renderChunk hook for files with type: "chunk": https://rollupjs.org/guide/en/#renderchunk. My conclusion is that we only miss something to transform files with type: "asset". My bet is on a new hook named renderAsset.

I know some naming recently changed to group assets and chunks but we still have a dedicated renderChunk hook and separated configs to name output assets and chunks:

• https://rollupjs.org/guide/en/#outputassetfilenames
• https://rollupjs.org/guide/en/#outputchunkfilenames

So I think @web/rollup-plugin-import-meta-assets is the right name but again, I won't block on this.

[daKmoR]

@LarsDenBakker there are no types right? why does the linting of types does not fail?

[hsablonniere]

I have no idea. I'm new here 😆
Is there another plugin source I can get inspiration from?

[daKmoR]

potentially here https://github.com/modernweb-dev/web/blob/master/packages/rollup-plugin-copy/src/copy.js

[hsablonniere]

Let me try this: https://rollupjs.org/guide/en/#thissetassetsourceassetreferenceid-string-source-string--uint8array--void

[hsablonniere]

Let me try this: rollupjs.org/guide/en/#thissetassetsourceassetreferenceid-string-source-string--uint8array--void

I tried to emit files without source, then I tried in a different plugin several hooks to see if I could set the source of an asset => failed. renderStart is too soon to be able to list assets and augmentChunkHash seems too late.

[daKmoR]

I think your points are fair enough so we can merge and iterate...

still types & windows support is probably a blocker 😅
what do you think?

[hsablonniere]

I think your points are fair enough so we can merge and iterate...

Well, I may have a proposition 😆

still types & windows support is probably a blocker 😅

Yeah sure, I asked helped for windows support as I'm not sure what's happening and regarding types, are we just talking about some JSDocs? Do you have examples in the modern web repo?

[hsablonniere]

What I just achieved:

• In this plugin: emit assets without source and populate a list of assets with absolute paths and Rollup reference ids.

• In another inline plugin, using the same list, iterate on it, read contents, apply "minification" if necessary and call this.setAssetSource().

• It "works" with a simple test (asset names and hashes are correct).

• Having a shared list between plugins seems very fragile.

• It seems like there's no way to replace the source. You must emit the file without and then call this.setAssetSource(). This means this plugin would be useless without another plugin to read files and maybe transform them.

I tried all the ideas I had to make it work right now with what we have but I cannot do better than this.

[hsablonniere]

Windows seems to fail because of string comparisons and end of line differences. I'm not sure how to fix those.

[hsablonniere]

I think I found out the root of the pb for Windows 😌

Doing some grep in Rollup's code, it does not seem to honor Node.js os.EOL. It always uses LF (\n) even of Windows. It think we could consider it as a Rollup bug but 🤷

Right now, the GitHub actions setup triggers the checkout with https://github.com/actions/checkout.
It seems like the git config of this setup has core.autocrlf enabled.

The effect is, when I use readFile with my "snapshots" to compare them with what Rollup outputs, they contain new lines as CRLF causing my tests to fail because the content is different.

We're not alone in this: actions/checkout#135

Comments suggest two ways of doing it. Adding steps in the action config like this seems better than adding a .gitattributes file.

[hsablonniere]

@daKmoR @LarsDenBakker

• Windows build works 🎉
• I also added some JSDoc on the plugin