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

fix(asset): respect assetFileNames if rollupOptions.output is an array #8561

Merged
merged 7 commits into from Jun 15, 2022

Conversation

inkyMountain
Copy link
Contributor

Fix #4628

Description

If build.rollupOptions.output is configured as array, the assetFileNames option will be ignored, instead using a default assetFileNames. This PR adopt the first assetFileNames option in the array, and give a warn if multiple assetFileNames return different values.
Most users encounter this bug while using @vitejs/plugin-legacy, which wraps the output option, change it from object to array.

Minimal reproduction
Steps:

  • run npm run build
  • pay attention to the built file - typescript.svg. It should be emmited to dist/assets/subdir, but actually in dist/assets.

Additional context

I create a vite config file vite.config-multiple-output.js in playground/legacy, and a corresponding npm script pnpm build:multiple-output. After running this script, a vite.svg should be emitted to dist/assets/subdir/vite.[some hash].svg, and a warn log colored with yellow should be observed.


What is the purpose of this pull request?

  • Bug fix

  • New Feature

  • Documentation update

  • Other

  • 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).

  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev patak-dev added the p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) label Jun 13, 2022
Comment on lines +357 to +371
const defaultAssetFileNames = path.posix.join(
config.build.assetsDir,
'[name].[hash][extname]'
)
// Steps to determine which assetFileNames will be actually used.
// First, if output is an object or string, use assetFileNames in it.
// And a default assetFileNames as fallback.
let assetFileNames: Exclude<OutputOptions['assetFileNames'], undefined> =
(output && !Array.isArray(output) ? output.assetFileNames : undefined) ??
// defaults to '<assetsDir>/[name].[hash][extname]'
// slightly different from rollup's one ('assets/[name]-[hash][extname]')
path.posix.join(config.build.assetsDir, '[name].[hash][extname]')
defaultAssetFileNames
if (output && Array.isArray(output)) {
// Second, if output is an array, adopt assetFileNames in the first object.
assetFileNames = output[0].assetFileNames ?? assetFileNames
}

Copy link
Member

Choose a reason for hiding this comment

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

Using the first output configured by the user is a better choice here. Should we consider outputting multiple assets according to array in lib mode? (Maybe it has nothing to related to this PR)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could keep working to improve this later. At least this PR fixes the plugin legacy issue that is very common. So i think we should merge and then work on another PR

Copy link
Member

Choose a reason for hiding this comment

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

BTW, lib mode is just asset output once. and chunk and entry will output multi bundle

@patak-dev patak-dev merged commit 4e6c26f into vitejs:main Jun 15, 2022
@odai-alali
Copy link

Hello @poyoho

This MR caused a problem in our Vite Config and I would like to understand a couple of things before creating an issue.

My Vite Config looks like this

export default defineConfig(() => {
  
  return {
    ...
    build: {
      outDir: "dist/fragments",
      assetsDir,
      rollupOptions: {
        input: {
          ...
        },
        output: [
          {
            entryFileNames: `${assetsDir}/fallback/[name].js`,
            chunkFileNames: `${assetsDir}/fallback/[name].js`,
            assetFileNames: `${assetsDir}/fallback/[name].[ext]`,
          },
          {
            entryFileNames: `${assetsDir}/[name].[hash].js`,
            chunkFileNames: `${assetsDir}/[name].[hash].js`,
            assetFileNames: `${assetsDir}/[name].[hash].[ext]`,
          },
        ],
      },
    },
  };
});

We had this config since we need a copy of our builds without hashed names. This has worked for us with vIte@v2.

Now the question, Why should output assetFileNames be ignored if it is an array, it's not really "respecting" rollup options.

I've checked your MR #8597 and it seems that the same warning is still there. Am I missing something here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug 🔨 An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

output.assetFileNames does not work when the project use @vitejs/plugin-legacy
4 participants