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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: export helpers to allow 3rd parties to emit assets #9693

Closed
wants to merge 4 commits into from

Conversation

timacdonald
Copy link
Contributor

@timacdonald timacdonald commented Aug 16, 2022

Description

When using Vite as the build tool for server side language templated views, such as Laravel Blade (or PHP, Ruby, etc), it can be useful to inject assets into the build that are not referenced in JS or CSS "entry points".

Imagine the following contrived Blade template in an application that does not include any built JS or CSS:

@php
    $logo = Arr:random([
       'resources/images/logo-a.png',
       'resources/images/logo-b.png',
    ]);
@endphp

<img src="{{ $logo }}">

Vite does not compile or know about this template, so Vite has no way of knowing about the two logos that the developer wants to bundle.

To allow applications that do not include build entry points to also utilise Vite as a build tool, I would like to propose that Vite export the suggested functions. Exporting these functions allows plugins (namely, the official Laravel plugin) to inject assets that are to be included in the build.

As an example of what this unlocks: in the Laravel plugin we could offer a new key for paths to include in the build:

export default defineConfig({
    plugins: [
        laravel({
            assets: [
                "resources/images/**",
                "resources/fonts/**",
            ],
        }),
        viteImagemin(),
    ],
})

The Laravel plugin may then internally use this emitFile function to add these to the build. Having access to these internal function allows us to ensure we stay inline with Vite naming conventions and also handle the naming as per the default or overridden config for filename ([hash], [name], etc) patterns.

{
    /* ... */
    async buildStart() {
        resolveAssetsFromConfig().forEach(asset => {
            const content = await fsp.readFile(
                path.resolve(resolveConfig.root, asset)
            )

            this.emitFile({
                type: 'asset',
                source: content,
                name: asset,
                fileName: assetFileNamesToFileName(
                    resolveAssetFileNames(resolvedConfig),
                    asset,
                    getHash(content),
                    content
                ),
            })
        }),
    },
}

Why not just use the static copy plugin?

As shown in an earlier example, including these in the actual build allows vite plugins such as viteImagemin to "just work". Images included in the Laravel plugin then benefit from the same side-effects as if the assets were simply referenced in JS files.

Why not just import into your JS

Our current solution is that we recommend people still have an app.js and import images with globing:

import.meta.glob(['../images/**', '../fonts/**'])

However we feel this is really weird workaround for apps that literally don't have any JS in their app, thus we would love to be able to improve this experience for our plugin users.

Additional context

  • errrmmmm. maybe have a look at the fact that I had to bump the bundle size substantially 馃檲
  • If you can think of another way to handle this that I haven't seen, please educate me 鉂わ笍

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • (n/a) Ideally, include relevant tests that fail without this PR but pass with it.

Comment on lines -263 to +264
* converts the source filepath of the asset to the output filename based on the assetFileNames option. \
* this function imitates the behavior of rollup.js. \
* converts the source filepath of the asset to the output filename based on the assetFileNames option.
* this function imitates the behavior of rollup.js.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter was complaining about the trailing backslash

Comment on lines -279 to +282
* @param assetFileNames filename pattern. e.g. `'assets/[name].[hash][extname]'`
* @param file filepath of the asset
* @param contentHash hash of the asset. used for `'[hash]'` placeholder
* @param content content of the asset. passed to `assetFileNames` if `assetFileNames` is a function
* @param assetFileNames - filename pattern. e.g. `'assets/[name].[hash][extname]'`
* @param file - filepath of the asset
* @param contentHash - hash of the asset. used for `'[hash]'` placeholder
* @param content - content of the asset. passed to `assetFileNames` if `assetFileNames` is a function
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter was complaining about the missing dash

@timacdonald timacdonald marked this pull request as ready for review August 16, 2022 06:35
@timacdonald timacdonald changed the title feat: export helpers to allow 3rd parties to dynamically inject assets feat: export helpers to allow 3rd parties to emit assets Aug 16, 2022
@timacdonald
Copy link
Contributor Author

hey @ElMassimo, have you come up with an elegant way of handling this in Ruby land that I might have missed?

@ElMassimo
Copy link
Contributor

ElMassimo commented Aug 18, 2022

@timacdonald Vite Ruby provides an additionalEntrypoints option, which by default includes images, fonts, and icons in the sourceCodeDir, and is fully configurable.

vite-plugin-ruby will fingerprint those assets and emit the corresponding files, adding them to a separate manifest-assets.json file which vite_ruby can use to reference them as needed when vite_asset_path is used.


A little history:

Until vite@2-beta.51, Vite supported asset entrypoints (accidentally, perhaps). When support was removed, I opened:

but they were closed as wontfix (hence, the approach in vite-plugin-ruby):

the manifest (...) strictly assumes entries are js chunks and assets are just emitted files that belongs to chunks.

That's no longer true, as vite@3 added back support for CSS entrypoints, which will be added to the manifest as well:


As Vite became more used in backend integrations, the manifest became more important than initially anticipated, so it looks like we are circling back to #1765.

From my point of view, it would be better if Vite processed any file provided in rollupOptions.input and added it to the manifest, which would ensure any fingerprinting and processing goes through the plugin pipeline. Vite would discard the "fake" JS chunks as needed, as it currently happens for CSS assets.

@timacdonald
Copy link
Contributor Author

Thank you so much for this insight. I agree that a first party solution would be lovely (although I understand we are both a little bias for making things work with Bbck-end projects as well as possible hehe 馃槆)

From what I can see, you have had to replicate all of the functionality that I'm proposing Vite export. If an end-user was to change the naming convention used in rollup.output.assetFileNames, your plugin would not currently respect it - or you'd have to re-implement it yourself.

So I can see a benefit for Vite ruby in also having the proposed functions exported as well (again, assuming that there isn't a first party solution). Would you agree?

@patak-dev
Copy link
Member

I think the backend integrations ecosystem in Vite has changed enough in the past year that there is merit in having a rediscussion, more now that the manifest includes non-js assets (that was one of the reasons Evan wasn't onboard with the idea #1765 (comment)). Exposing all the needed helpers may be more problematic than providing a first-class solution.

@ElMassimo @timacdonald we're going to set up a vitejs/rfcs repo to formalize the RFC process. I think an RFC could be a good way to get the different parties involved and propose this Vite feature.

@ElMassimo
Copy link
Contributor

ElMassimo commented Aug 19, 2022

Exposing all the needed helpers may be more problematic than providing a first-class solution.

Agreed, exposing the helpers in the public API would make any internal change a potential breaking change.

a first party solution would be lovely

Happy to chime in if @timacdonald likes the idea of opening an RFC for this, it would benefit all backend integrations.

@timacdonald
Copy link
Contributor Author

Hey folks,

If that is how we wanna move forward - I'm more than happy to whip something up. Should I open a PR against https://github.com/vitejs/rfcs and are there any guidelines or anything?

Should I just take a look at the Vue RFC stuff and try to replicate that in the Vite repo?

@patak-dev
Copy link
Member

Hey @timacdonald, Anthony has been working today to set up the vitejs/rfcs repo. We are going to use something similar to Vue, although with some simplifications. There are already docs there in the Readme that you can follow. I think what we are missing is some automation to auto-create a discussion after a PR when needed, but you can do it by hand for now.

@antfu
Copy link
Member

antfu commented Aug 26, 2022

@timacdonald The RFC repo is ready, and you can follow the instruction on the repo readme. Looking forward to seeing your RFC!

@sapphi-red
Copy link
Member

sapphi-red commented Nov 25, 2022

Closing as #10909 removed assetFileNamesToFileName and resolveAssetFileNames and this PR doesn't make sense any more.

@timacdonald When fileName is omitted from this.emitFile, the file name will be resolved by rollup.output.assetFileNames. Maybe you could just omit fileName like this?

{
    /* ... */
    async buildStart() {
        resolveAssetsFromConfig().forEach(asset => {
            const content = await fsp.readFile(
                path.resolve(resolveConfig.root, asset)
            )

            this.emitFile({
                type: 'asset',
                source: content,
                name: asset, // maybe this should be `path.basename(assets)` instead
            })
        }),
    },
}

@sapphi-red sapphi-red closed this Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants