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

Improve and fix asset emission #2796

Merged
merged 9 commits into from Apr 11, 2019
Merged

Improve and fix asset emission #2796

merged 9 commits into from Apr 11, 2019

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This will resolve some important issues when emitting assets and make it more configurable at the same time. The most important fix is that for some formats, generated asset URLs would point to a file in the parent directory instead of the actual asset. Also, the generated code for asset URLs has been refined, similar to the refinements for import.meta.url in #2785:

  • all formats except cjs and umd assume the code runs in a browser environment where URL and document are available
  • for cjs and umd, presence of a browser environment is detected by checking for document (after which presence of URL is assumed)

Generated code:

// amd
new URL(module.uri + '/../relative/path/to/asset.svg', document.baseURI).href

// cjs
(typeof document === 'undefined' ? new (require('u' + 'rl').URL)('file:' + __dirname + '/relative/path/to/asset.svg').href : new URL((document.currentScript && document.currentScript.src || document.baseURI) + '/../relative/path/to/asset.svg').href)

// es
new URL('relative/path/to/asset.svg', import.meta.url).href

// iife
new URL((document.currentScript && document.currentScript.src || document.baseURI) + '/../relative/path/to/asset.svg').href

// system
new URL('relative/path/to/asset.svg', module.meta.url).href

// umd
(typeof document === 'undefined' ? new (require('u' + 'rl').URL)('file:' + __dirname + '/relative/path/to/asset.svg').href : new URL((document.currentScript && document.currentScript.src || document.baseURI) + '/../relative/path/to/asset.svg').href)

To be able to control and optimize this code, there is now a new plugin hook

resolveAssetUrl: ({assetFileName: string, relativeAssetPath: string, chunkId: string, moduleId: string, format: string}) => string | null

@lukastaegert lukastaegert changed the title Improve asset emission Improve and fix asset emission Apr 8, 2019
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

I can't say I've done a thorough review, but the hook seems useful, and that it fixes what sounds like a critical bug seems pretty important. Great work.

@lukastaegert lukastaegert force-pushed the improve-asset-emission branch 2 times, most recently from 13b0d25 to ddfb76e Compare April 11, 2019 06:14
@lukastaegert lukastaegert changed the base branch from refactor-import-meta to master April 11, 2019 06:29
@lukastaegert lukastaegert merged commit 9ffe6f5 into master Apr 11, 2019
@lukastaegert lukastaegert deleted the improve-asset-emission branch April 11, 2019 06:42
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

2 participants