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(url): add asset module support (`url`) #6446

Open
wants to merge 6 commits into
base: next
from

Conversation

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Feb 6, 2018

This PR attempts to add support for a new module type (module.type === 'url/experimental') to handle 'assets' (Images, Fonts, SVG, ...) 'natively', replacing the need for loaders like the url/file-loader

webpack.config.js

const config = {
  output: {
    urlModuleFilename: "[modulehash].[ext]"
  },
  module: {
    rules: [
       {
           test: /\.png$/,
           type: 'url/experimental'
       },
       {
           test: /\.svg$/,
           type: 'url/experimental'
       },
       ...
    ]
  }
}

Status


Depends on #8109

Fix

  • Correct module.source generation (URLJavascriptGenerator.js)
  • [x] Rule defaults for type: 'url' (WebpackOptionsDefaulter.js)
    • [x] Images, Vectors (e.g test: /\.(ico|png|jpg|jpeg|gif|svg|webp)(\?v=\d+\.\d+\.\d+)?$/i)
    • [x] Fonts (e.g test: /\.(eot|ttf|woff|woff2)(\?v=\d+\.\d+\.\d+)?$/i)

Feature

  • [x] Add parser options (TBD)
    • limit (inline asset as (base64 encoded) if size < limit)
    • optimize (register an asset optimizer like e.g sharp) [Better done via a loader]
  • [x] Add generator options (TBD)
  • [x] [file] && [basename] placeholder support (config.output.urlModuleFilename) (#8109)
  • preload Support (partially depending on #6447) (MDN Link)
    • <link rel="preload" href="/path/to/image.png as="image">
    • <link rel="preload" href="/path/to/font.woff as="font">
  • prefetch Support (partially depending on #6447) (MDN Link)
    • <link rel="prefetch" href="path/to/big.png">

Chore

  • Add Type Declarations

Test

  • Add Tests

@TheLarkInn TheLarkInn added this to the webpack 4.x milestone Feb 6, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch 3 times, most recently from 22c31a6 to 66167b9 Feb 7, 2018

@sokra sokra changed the base branch from next to master Feb 13, 2018

@sokra sokra removed the PR: next label Feb 13, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch from 66167b9 to 4c900d8 Mar 3, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 6, 2018

/cc @sokra what do you think about this implementation? It is allow avoid to use file-loader, also a lot of configurations have file-loader as fallback for unknown assets, this solution allow to avoid this too.

@webpack-bot

This comment has been minimized.

Copy link

webpack-bot commented May 16, 2018

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

@sokra sokra modified the milestones: webpack 4.x, webpack 5 Jun 21, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch from 4c900d8 to 5780371 Sep 21, 2018

@michael-ciniawsky michael-ciniawsky changed the base branch from master to next Sep 21, 2018

@michael-ciniawsky michael-ciniawsky changed the title [WIP] feat(url): add assets support (`url/experimental`) [WIP] feat(url): add assets support (`url`) Sep 21, 2018

@michael-ciniawsky michael-ciniawsky added this to Yes (and in progress) in webpack 5 Sep 21, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch 2 times, most recently from fa7f777 to 8cdacfb Sep 21, 2018

@webpack webpack deleted a comment from webpack-bot Sep 22, 2018

@michael-ciniawsky michael-ciniawsky self-assigned this Sep 22, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch 4 times, most recently from 5fc4baf to 9ac7fde Sep 22, 2018

@@ -113,6 +113,32 @@ class WebpackOptionsDefaulter extends OptionsDefaulter {
}
return "[id].js";
});
this.set("output.moduleFilename", "make", options => {

This comment has been minimized.

@sokra

sokra Sep 27, 2018

Member

Maybe we should use output.urlModuleFilename similar to output.webassemblyModuleFilename.

Both could be default to output.moduleFilename which defaults to "[modulehash].[ext]".

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Sep 27, 2018

Member

Yes, either a 'top-level' output option for each type (for moduleFilename (${type}moduleFilename)) is still needed here (e.g to support specifying different subdirectories per type like images/[modulehash].[ext]) or the output 'top-level' gets grouped per type (maybe a good idea in regards to HTML/CSS/* Modules in the future, but this can be addressed later (also not sure about it))

webpack.config.js

const config = {
   output: {
       js: {
         filename: '[id].[name].[hash].[ext]',
         chunkfilename: '[id].[name].[hash].[ext]'
      },
      css: {
         filename: '[id].[name].[hash].[ext]',
         chunkfilename: '[id].[name].[hash].[ext]',
         moduleFilename: '[id].[name].[hash].[ext]',
      },
      url: {
         filename: '[id].[name].[hash].[ext]' // used as moduleFilename
         modulefilename: '[id].[name].[hash].[ext]',
      }
   }
}

But this way it's also not 💯 sound e.g output.(url|wasm).filename && especially output.(url|wasm).chunkFilname wouldn't make any sense at all

This comment has been minimized.

@evilebottnawi

evilebottnawi Sep 27, 2018

Member

Without test/includes/exclude it is doesn't make sense, a lot of people use multiple url/file-loader with difference test/include/exclude. Maybe better:

const config = {
   output: {
      {
         test: /\.(png|gif|svg)$/i,
         include: /src/,
         filename: 'app/[id].[name].[hash].[ext]',
         chunkfilename: 'app/[id].[name].[hash].[ext]'
      },
      {
         test: /\.(png|gif|svg)$/i,
         include: /node_modules/,
         filename: 'vendor/[id].[name].[hash].[ext]',
         chunkfilename: 'vendor/[id].[name].[hash].[ext]'
      }
   }
}

Same for js/css/etc

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Sep 27, 2018

Member

Would test|include|exclude not still be handled by module.rules (those apply at a different phase/stage)? The basic idea is

output: {
   [type]: {
     // 1. Common options (filename[chunkFilename [, moduleFilename]])
     // 2. Common placeholders (`[id]`, `[name]`, `[hash]`, `[ext]`)
     // e.g 
     filename: '[name].[hash].[ext]'
   },
   ...types
}

But this needs more discussion

This comment has been minimized.

@evilebottnawi

evilebottnawi Sep 27, 2018

Member

@michael-ciniawsky basic idea not cover when you want to move difference assets to difference directories (example - assets from node_modules to build/vendor/assets and assets from app to build/app/assets), many people do this, i don't know why, but they do.

@@ -104,6 +107,10 @@ const replacePathVariables = (path, data) => {
: module.renderedHash || module.hash);
const moduleHashWithLength =
module && "hashWithLength" in module && module.hashWithLength;
// TODO(michael-ciniawsky)
const moduleExtension = module && module.rawRequest

This comment has been minimized.

@sokra

sokra Sep 27, 2018

Member

Extract the extension for data.filename.

@@ -139,6 +146,7 @@ const replacePathVariables = (path, data) => {
)
.replace(REGEXP_ID, getReplacer(chunkId))
.replace(REGEXP_MODULEID, getReplacer(prepareId(moduleId)))
.replace(REGEXP_MODULE_EXT, getReplacer(moduleExtension))
.replace(REGEXP_NAME, getReplacer(chunkName))
.replace(REGEXP_FILE, getReplacer(data.filename))
.replace(REGEXP_FILEBASE, getReplacer(data.basename))

This comment has been minimized.

@sokra

sokra Sep 27, 2018

Member

basename doesn't need to be passed here but could be extracted from data.filename.

}),
filenameTemplate,
pathOptions: {
chunkGraph,

This comment was marked as resolved.

@sokra

sokra Sep 27, 2018

Member

add filename: module.resource

This comment was marked as resolved.

@sokra

sokra Sep 27, 2018

Member

Do the same in WebassemblyModulesPlugin to add support for [ext].

module.exports = {
mode: "development",
output: {
moduleFilename: "images/file.[ext]"

This comment has been minimized.

@sokra

sokra Sep 27, 2018

Member

You should also be able to use [basename] and [file] too

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Sep 27, 2018

Member

[basename] or [name] ? [name] doesn't currently work as it is 'reserved' for chunks and not available in this context, the question is if it would be possible to support common placeholders like [id], [name], [hash], [ext] for each context and scope them if (isModuleFilename) '[hash]' === '[modulehash]' and so on (but this can be considered later, I will the address the other comments first). I tried [file] but it didn't work (I will look into it) :)

@@ -84,7 +84,7 @@ class NormalModule extends Module {
this.request = request;
this.userRequest = userRequest;
this.rawRequest = rawRequest;
this.binary = type.startsWith("webassembly");
this.binary = /(url|webassembly)/.test(type);

This comment has been minimized.

@Janpot

Janpot Sep 27, 2018

Member

What about something like

/^(url|webassembly)\b/.test(type)

?

'url': true
'xurl': false
'urlx': false
'url/x': true
'webassembly': true
'xwebassembly': false
'webassemblyx': false
'webassembly/x': true

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Oct 2, 2018

Member

We only have e.g url (future) and url/experimental (present) as valid module.type the / is kind of 'enforced' here to specify the subtype (if any)

@@ -1014,6 +1014,11 @@
"description": "If `output.libraryTarget` is set to umd and `output.library` is set, setting this to true will name the AMD module.",
"type": "boolean"
},
"urlModuleFilename": {
"description": "The filename of modules as relative path inside the `output.path` directory.",

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Sep 27, 2018

Member

needs to be reworded

@@ -0,0 +1,5 @@
import url from "./file.png";

it("should output asset into assets/", () => {

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Sep 27, 2018

Member

needs to be reworded

@@ -0,0 +1,16 @@
module.exports = {
mode: "development",
devtool: false,

This comment has been minimized.

@michael-ciniawsky

@michael-ciniawsky michael-ciniawsky removed this from Yes (and in progress) in webpack 5 Oct 1, 2018

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch from fc69635 to 5658885 Oct 2, 2018

@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented Oct 2, 2018

https://dev.azure.com/webpack/webpack/_build/results?buildId=257&view=logs

Aren't hashes normally stable across platforms? Also

  • Azure
  • Appveyor

is kind of weird... maybe different windows versions...

@webpack-bot webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Oct 2, 2018

@webpack-bot

This comment has been minimized.

Copy link

webpack-bot commented Oct 2, 2018

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@@ -5,6 +5,8 @@

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Oct 2, 2018

Member

Amend/Drop chnages in favor of #8109

chunkGraph
});

const source = `module.exports = __webpack_require__.p + '${url}'`;

This comment has been minimized.

@michael-ciniawsky

michael-ciniawsky Oct 2, 2018

Member

Still not sure if this handels all possible cases... :)

"use strict";

class UrlParser {
parse(source, state) {

This comment was marked as resolved.

@michael-ciniawsky

michael-ciniawsky Oct 2, 2018

Member

Could this be empty or does it need to return ?

@michael-ciniawsky michael-ciniawsky force-pushed the feature/url branch from 6a5412b to d27d66a Oct 2, 2018

@ianstormtaylor

This comment has been minimized.

Copy link

ianstormtaylor commented Nov 19, 2018

Should this be called asset (or file) instead of url? It seems a bit strange for the loader configuration to dictate the name of the thing. Calling it urlModuleFilename seems to me like it's going to be hard for people to understand, whereas assetModuleFilename makes slightly more sense.

Also, just a thing to consider... It might be nicer for filename to support a dictionary of type -> filename, instead having to introduce the awkward *ModuleFilename option for each type of module in the future. Especially if Webpack ever has plans for extendible "types". (Don't know much about this.)

{
  output: {
    filename: {
      html: '[name].[ext]',
      css: '[name]-[hash].[ext]',
      asset: '[hash].[ext]',
      ...
    }
  }
}
@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented Nov 20, 2018

Should this be called asset (or file) instead of url? It seems a bit strange for the loader configuration to dictate the name of the thing. Calling it urlModuleFilename seems to me like it's going to be hard for people to understand, whereas assetModuleFilename makes slightly more sens

None of those are really ideal for output imho. For the module.type url or file is ok/fine

Also, just a thing to consider... It might be nicer for filename to support a dictionary of type -> filename, instead having to introduce the awkward *ModuleFilename option for each type of module in the future. Especially if Webpack ever has plans for extendible "types". (Don't know much about this.)

Agreed but this is not how it currently works and therefore this would require adding breaking changes to the PR 🙃. output currently distguishes between the different 'asset' types (Roughly Chunk (Sync) output.filename, Chunk (Async) output.chunkFilename, Module (module.type) => output.[type]ModuleFilename). To my knowledge the sole reason for adding a [type]ModuleFilename property per module.type instead of just a single moduleFilename (at least) is to support subdirectories per type

output: {
  urlModuleFilename: 'a/[name].[ext]',
  wasmModuleFilename: 'b/[name].[ext]'
}

But this could be solved in other ways... e.g I like your example sans [ext] (not needed then) and
instead of using solely the module.type e.g url|file|asset as key for filename, this could be made 'smarter' by matching the filename[key] with the resource extname

{
  output: {
    filename: {
      <ext|type>: <template>,
      html: '[name]', // 'index.html' Entrypoint|Chunk|Module { type: 'html', ...}
      css: '[name].[hash]', // 'index.6r347849.css' Entrypoint|Chunk|Module { type: 'css', ...}
      png: '[hash]', // '4836573.png' Module { type: 'url', resource: 'src/images/file.png' }
      jpg: '[id].[hash]' // '1.4ftjk3.svg' Module { type: 'url', resource: 'src/images/file.jpg' }
      svg: 'dir/[name]' // 'dir/file.svg' Module { type: 'url', resource: 'src/icons/file.svg' }
      wasm: '[id]' // '10.wasm' Module { type: 'webassembly', resource: 'src/wasm/module.wasm' }
    }
  }
}

¯_(ツ)_/¯

@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented Nov 20, 2018

That's likely also not the whole story... minified? compressed? etc... e.g

{
  output: {
    filename: {
     <ext|type>: {
       <template>
       <variant>+
     },
     css: {
       template: '[name].[hash]',
       minified: true,
       compressed: true
     }
  }
}
Source: index.7483rt754.css
Minified: index.62fzzvwzr.css
Compressed: index.62fzzvwzr.css.gz

But that's not a good API

@ianstormtaylor

This comment has been minimized.

Copy link

ianstormtaylor commented Nov 20, 2018

Fair enough! You know more about the internals than I do, just wanted to point out a potential spot that would be awkward and wouldn’t be very extensible in the future so that it doesn’t get overlooked! Thanks for working on these new module types!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment