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(build): respect rollup output.assetFileNames, fix #2944 #4352

Merged
merged 9 commits into from Aug 2, 2021

Conversation

SegaraRai
Copy link
Contributor

Description

Fixes #2944.
This PR is a continuation of #2957.

I don't know if it's ok to create a continuation of someone's work, so please close this if it is invalid.

Additional context


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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

Thanks for the PR @SegaraRai, it is ok to continue the work of others if it got stale. No problem.

We should also move the 233-300 block to a new function at the end of this file, so the main function is easier to follow. And we can replace the naming of vars like _extname to extname there.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 22, 2021
@SegaraRai
Copy link
Contributor Author

Thanks for the review.
Fixed the code.

BTW, I think I should write some tests, but I couldn't figure out how to write tests with multiple vite.config.js...

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

As you mentioned, beside of missing test, LGTM

patak-dev
patak-dev previously approved these changes Jul 22, 2021
@patak-dev
Copy link
Member

There were some tests added here #4122 for plugin-legacy, but I don't think that this is the best way to test it. We now have 404 in that playground for example. IMO we need a better way to add this to the test suite. We could do it in another PR also, maybe changing some of the playgrounds (like the asset one) to use the function type and return different patterns and then check that the files were properly generated? We could also add unit tests for the new fileAssetsNameToFileName function (check out other __tests__ folders in src)

@SegaraRai
Copy link
Contributor Author

Thanks. I've added some unit tests.
I'm sorry but the test using vite.config.js was bit difficult for me, so I'm going to give up adding it.

@jbruni
Copy link
Contributor

jbruni commented Jul 29, 2021

👀 Expecting this to be released.

Thank you.

@patak-dev patak-dev changed the title fix(build): respect rollupOptions.output.assetFileNames, fix #2944 fix(build): respect rollup output.assetFileNames, fix #2944 Aug 2, 2021
@patak-dev patak-dev merged commit cbd0458 into vitejs:main Aug 2, 2021
@JR93
Copy link

JR93 commented Aug 16, 2021

It still does not work when I use the plugin @vitejs/plugin-Legacy. The plugin seems to add an output config, so the output option is an array type, then I always get the defaults value.

const output = config.build?.rollupOptions?.output
const assetFileNames =
  (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]')

@SegaraRai
Copy link
Contributor Author

output of array form is currently not supported because it is not clear which one to use.
This could be fixed in another PR if there is an appropriate logic...

@patak-dev
Copy link
Member

Yes, @JR93 please open a new issue with a minimal reproduction so the logic can be discussed there. Thanks!

lucsky added a commit to lucsky/vite-plugin-svgr that referenced this pull request Aug 20, 2021
It looks like ViteJS 2.5.0 fixed a compatibility problem with Rollup (probably this: vitejs/vite#4352) wich makes bundle entries to have a correct name, which drastically simplify the process of finding a corresponding transformed modules.
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
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.

rollupOptions.output.assetFileNames does not work
6 participants