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

feat: build library new option - emitAssets #5028

Closed
wants to merge 5 commits into from

Conversation

torsteinringnes
Copy link

Fix: [#4454, #3295]

Description

If your environment disables you from using script modules, you have to use lib:true to build. This forces assets like jpg/png to be encoded inline which causes bundle files to be huge.

This PR introduces a new option emitAssets:boolean, by default assets will remain inline when not setting emitAssets.

There are 2 relevant issue:
#3295
#4454

To test use build.lib.emitAssets:true in the vite.config.js.
Expected results: Imported files like .svg,.png.jpg are emitted and referenced by their path in the js bundle.

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.

@Shinigami92 Shinigami92 added p2-nice-to-have Not breaking anything but nice to have (priority) needs test labels Sep 22, 2021
@torsteinringnes
Copy link
Author

@Shinigami92 Provided a couple of tests :)
Test 1: Assets when building lib without setting emitAssets remain inlined.
Test 2: emitAssets:true actually emits assets

@patak-dev
Copy link
Member

@hronro would you help to review this PR? @torsteinringnes IIUC from the linked issue the idea was to apply this only when the output format is es, no? Also, I don't see custom handling to use URL(import.meta.url, '...') for these assets. Maybe I'm missing some context in how this would be used

@torsteinringnes
Copy link
Author

All it does is emitting image assets like with default build. I dont see why it only should be in es , so its likely me missing some context on how it should be used :/

@hronro
Copy link
Contributor

hronro commented Oct 11, 2021

@torsteinringnes
In default build, vite will assume you are building an application(not library), and the output will be loaded in an ESM compatible environment (like all the modern browsers), so the output will contains something like new URL(import.meta.url, '...'), which is only valid in an ES module context.

However, in library mode, people may want to target different module systems (esm, commonjs, or whatever), and vite also allows people to do that. Obviously, new URL(import.meta.url, '...') is not valid in any module system other than ESM. This is why in the issue #4454, I said we should only emit assets when format is es. (Honestly, I don't know if there is actually a way to import assets in JavaScript in a library which targets commonjs, since publicPath is not working for a library.)

@torsteinringnes torsteinringnes deleted the fix-#3295 branch November 3, 2021 21:25
@realityfilter
Copy link

We also need this feature. What was the problem with this pull request so it got closed? Is an additional check for es output sufficient for approval?

@Bigfish8
Copy link
Contributor

Bigfish8 commented Dec 3, 2021

@hronro
I dont know why should must use new URL() here.
For example:

import foo from './image.png';

const bar = foo; // 1.new URL(import.meta.url, './assets/image.2d8efhg.png').href 2.'./assets/image.2d8efhg.png'

Is there some different for 1.new URL()call and 2.relative path here for lib mode?

@hronro
Copy link
Contributor

hronro commented Dec 3, 2021

Of course there is.

Basically for applications (not libraries), we can use absolute path (please note relative path will not work), and this is why webpack's publicPath option exist.

However for libraries, we have no ideas what the publicPath will be (it depends on library consumers rather than library authors), so we can not use it for libraryies.

@franktopel
Copy link

franktopel commented Dec 5, 2021

Without the ability to exclude binary assets like images, fonts etc. from being inlined in bundled code, there is no way to create library output compliant with security-conscious CONTENT-SECURITY-POLICY settings, in which you definitely don't want to allow data: sources, and hashes or nonces only work for scripts and styles, not for binary resources.

So using library mode in Vite currently enforces security holes with regard to CSP/cross-site scripting. This effectively rules out Vite.js as a build stack for security-sensitive environments.

However for libraries, we have no ideas what the publicPath will be (it depends on library consumers rather than library authors), so we can not use it for libraryies.

The least thing that imo is an absolute necessity here is to allow asset inlining to be excluded from the generation of the CSS bundle. Instead of inlining those assets, allow an option to transform paths for excluded assets from DEVELOPMENT to PRODUCTION.

@hronro
Copy link
Contributor

hronro commented Dec 6, 2021

@franktopel What do you mean by "allow an option to transform paths for excluded assets from DEVELOPMENT to PRODUCTION"?

@franktopel
Copy link

franktopel commented Dec 6, 2021

When assets are no longer inlined, we probably need an option to tell Vite that what is /src/images/ in development, is /static/assets/img/ in production (just an example). We should not require library consumers to mimic our development folder structure.

@hronro
Copy link
Contributor

hronro commented Dec 6, 2021

@franktopel

We should not require library consumers to mimic our development folder structure.

I totally agree with that.

In the original issue ( #4454) I post, I said this is only for es format, because I assume the library will be served directly in a HTTP server (or on the CDN), or the consumer's build tool understand the new URL(import.meta.url, '...') syntax, so the consumer don't have to know the file structure of library.

For other formats, I'm not sure if there is a way to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants