Proposal: explicit named imports for non-JS/CSS assets

gaearon commented 6 years ago

Problem

We currently allow you to do this:

import logo from './logo.png';

After getting used to it, you’ll probably be comfortable with this giving you a URL.

But what about other types? For example, what should this return?

import doc from './doc.md';

Markdown source? Compiled HTML? An AST?

What about this?

import Icon from './icon.svg';

Should this give you a link? The SVG content? A React component?

The usual answer is “decide it for yourself in the configuration file”. However, that doesn’t work for CRA so we decided to treat all unknown extensions as URL imports. This is not ideal because in some cases it just doesn’t make any sense, and in others there are advanced (but still relatively common) use cases that aren’t satisfied.

Proposal

What if we allowed to user to pick what they want, from a limited supported subset per filetype?

import { url as logoUrl } from './logo.png';
import { html as docHtml } from './doc.md';
import { ReactComponent as Icon } from './icon.svg';

Named imports are checked by webpack so you’d get a compile error if you use an unsupported one.

Things that are unused will be tree shaken so if you only use e.g. HTML of Markdown files, their source won’t be bundled. Same for SVGs (whether you consume them as raw source, URLs, or React components).

Other zero-configuration tools can also adopt this approach.

Concerns

What do we do with the default import? Ideally I’d like to forbid it for anything other than JS/CSS because the intent is not clear for asset files (which version do you get?) We could do this with a lint rule.
If we make the breaking change, how do we update the consumers? We could write a codemod (it should be very simple).
It would be nice to coordinate this across at least a few other projects (e.g. @ndelangen Storybook). Maybe @KyleAMathews (Gatsby) @devongovett (Parcel) @rauchg (Next) would also be interested? I imagine we’ll need to write a multi-file Webpack loader for this, but I don’t see why other bundlers couldn’t adopt a similar convention.
Build performance: generating all possible content variations can be too slow. Ideally it would be nice if loaders had information about which import was used.

Thoughts?

gregberge commented 6 years ago

I think it is a very good thing to adopt a convention on it.

Even without talking about zero-configuration, today we can't use several Webpack loaders safely. We had the problem with svgr + url-loader. I made a workaround to solve it but it should not be a workaround.

I think we should add @sokra in this discussion, Webpack could force it by design.

dmitriid commented 6 years ago

What do we do with the default import? Ideally I’d like to forbid it for anything other than JS/CSS because the intent is not clear for asset files (which version do you get?)

We may want to define the rules for the default as and extension of "pick what they want, from a limited supported subset per filetype".


import { url } from './logo.png'

import Logo from './logo.png'

Logo.url === url

Will this work in practice though?

gaearon commented 6 years ago

I think we should add @sokra in this discussion, Webpack could force it by design.

I think this needs to be a grassroots effort because enforcing anything like this from webpack’s side is going to take a super long time, even if they do it.

gaearon commented 6 years ago

@dmitriid

My thinking is that your example shouldn't build because it has a default import (import Icon). I think that if we adopt this approach we should just forbid default import and ask people to be explicit about what they want.

We could maybe keep default import for compatibility reasons for a while.

Munter commented 6 years ago

If you want to foster a culture of zero config tooling, then it would be nice to come up with a convention that doesn't break module imports by default. Using the import statement for anything non-module is already forcing a specific tool chain dependency and thus a default config requirement on the developer.

I'd really love to see a syntax that was actually JS compatible by not overloading the import statement

gaearon commented 6 years ago

@Munter I understand this sentiment but it’s not very practical in my experience. We’re trying to do the best with what we got. FWIW, my proposal is actually closer to no lock-in than syntax like raw!./file.md because at least it is possible to auto-generate file.md.js that contains those exports. So in practice it can even work on Node if you have a codegen step.

Let’s not derail this thread with a general discussion whether import for assets is a good idea. The fact is that it solves many real use cases, and people want to do it. So the question here is how to make that more ergonomic and less confusing.

gaearon commented 6 years ago

I think in order to adopt this we’ll have to keep supporting default import for a while. This will make it possible for tools like Storybook to catch up. Then we can decide whether we want to deprecate support for the default import or not.

dmitriid commented 6 years ago

@gaearon

I think I agree with you in principle, but I think there are potential downsides. Though as I'm writing them down, I feel like I might be wrong about some of my assumptions.

import * as Icon from 'icon.svg'

In the absence of a default export devs will still attempt the above, which will lead to multiple questions similar to "Why import * as React from 'react' and not import React from 'react'".

Why would dev need * from an svg? Because sometimes they just do :)

assumption that a default export is just an object with exposed/exported properties

Even though this assumption is incorrect due to how import/loading works, many devs (including me :) ) will assume the following code to be equivalent:


import {prop} from 'something';

// many will assume it to be equivalent to

import Obj from 'something';
const prop = Obj.prop

It doesn't help that a lot of transpiled code looks that way.

ndelangen commented 6 years ago

My first proposal would be to create a multi-markdown-loader that would export all the formats we want. and it could even have a default export. What this default would be exactly, I don't know.

I also see potential for this idea to take hold for other files then markdown.

import { url } from './my.svg';
import { raw } from './my.txt';

gaearon commented 6 years ago

Note there’s also a build performance concern: https://mobile.twitter.com/wSokra/status/950713344163446785.

ranyitz commented 6 years ago

@gaearon, That sounds like an awesome idea!

Supporting the default imports would only be less performant (from bundle size, and compile time aspects) and as you said, a lint rule could do the job for the depreciation period.

About the build performance problem, the loader will process all the possibilities, usually with no need, That might increase the build time. Ideally, we would like the loader to be triggered by a combination of the extension and the named export, for example:

svg + url => use url-loader

import { url } from './logo.svg';

svg + ReactComponent => use react-svg-loader

import { ReactComponent } from './logo.svg';

I understand that there is a problem to do it in webpack because we only get what the user imported after the loaders ran.

When using static import it should be possible to get that information at an earlier stage.

@sokra, is there a plan of doing something like this?

gaearon commented 6 years ago

Unfortunately @sokra says it’s not easy due to the current architecture: https://mobile.twitter.com/wSokra/status/950728052442529793

viankakrisna commented 6 years ago

maybe it can be implemented as a babel-plugin?

import { raw } from './file.md';

becomes

import raw from 'raw!./file.md';

I think i've seen similar approach (rewriting imports) with babel-plugin-lodash. Can it be applied to loaders too?

Andarist commented 6 years ago

@viankakrisna it could, from the babel's perspective this is just a js syntac and it could transpile it to the custom loader format (import raw from 'raw!./file.md';)

devongovett commented 6 years ago

Yep, Parcel has had requests to get the URL for officially supported formats like JSON instead of inlining into the JS bundle.

The proposed syntax is potentially a bit problematic I think. Should it work for JS imports as well? i.e. import {url} from './my-script.js'. There would be no way to know statically whether url refers to a legit JS export or the URL to the file produced by the bundler, since the file extension is optional to imports.

In Parcel, and I think in webpack too, resolution of dependencies happens later - after loaders have already run. So in order to implement this syntax the loader would need to do its own resolution to check if it's a JS file. Also, I don't think we should treat JS files specially - you should be able to import a URL to a JS file as well.

gregberge commented 6 years ago

@devongovett the problem is already present with JS files. If I apply url-loader on JS files, I can't access the JS code itself any more.

But thinking about the JS file case, I don't know how to solve this issue. It seems more complicated than it appears.

FWeinb commented 6 years ago

@viankakrisna Can be done with a babel-plugin, quick test here.

sompylasar commented 6 years ago

import { [Symbol.url] as jsUrl } from './foo.js'; — can this work? Getting a JS URL is a rare case if we have dynamic import().

RE: loaders do their own resolving, if resolving is exposed as a function (memoized somehow?), it can be reused from a loader.

yyx990803 commented 6 years ago

For the record, I'm interested in implementing this is the new vue-cli default configuration.

I was about to propose doing this via a babel-plugin but someone beat me to it :)

I think babel plugin is a good idea:

applicable to any file type, no need to implement a mega-loader or different loaders for different file types.
no tree-shaking needed
no perf concerns

What @FWeinb showed is already promising.

Note: the babel plugin is webpack-specific, implementation for Parcel probably has to be done in a completely different fashion.

bmeck commented 6 years ago

Is there any problems with doing this via a URL Query Parameter so that it could also function in other places?

stereobooster commented 6 years ago

Also want to let you know about another potential use case (from svelte)

import MyComponent from './MyComponent.html';

sompylasar commented 6 years ago

Is there any problems with doing this via a URL Query Parameter so that it could also function in other places?

If by URL Query Parameter you mean something like import foo from './foo.svg?ReactComponent' (altering the module request syntax) then it's as not standardized and webpack-specific as import foo from 'react-svg-loader!./foo.svg'.

Having a set of named exports from modules is not standardized as well, but may be easier to standardize across bundlers, also may be easier to provide type definitions.

bmeck commented 6 years ago

@sompylasar Query Parameters are supported by browsers and servers, I'm not sure how this specific name list style of solution could be supported in those environments.

sompylasar commented 6 years ago

@bmeck But we're talking about bundling assets together here, not loading a single asset into the browser.

bmeck commented 6 years ago

@sompylasar so why be incompatible?

sompylasar commented 6 years ago

@bmeck Please explain your approach with specific code examples. Thank you.

bmeck commented 6 years ago

@sompylasar you can grab all the items as properties required by this proposal with the following syntax:

import logoUrl from './logo.png?as=url';

?as=url is visible both ahead of time and well understood by both clients and servers as a ["as","url"] pair. Instead of having a special interpretation of how named imports work that is only visible ahead of time/with special tooling, a person could write a server that understands the query parameter for things such as development modes that do not demand bundling repeatedly.

egoist commented 6 years ago

For webpack, seems a loader would do the job:

import { html, url } from '!multi-format-loader?format=html,url!./foo.md'

!multi-format-loader?format=html,url!./foo.md basically returns:

export var html = '...html'
export var url = require('!file-loader!./foo.md')

The webpack config would look like:

const options = {
  html: {
    test: /\.md$/,
    process(content) {
      return toHTML(content)
    }
  },
  url: {
    test: /\.md$/,
    process(content, resource) {
      return `require('!file-loader!${resource}')`
    }
  }
}

module.exports = {
  module: {
    rules: [
      {
        loader: 'multi-format-loader',
        options
      },
      {
        loader: 'babel-loader',
        test: /\.js$/,
        options: {
          plugins: [
            ['multi-formar-loader/babel', { ext: ['.md'] }]
          ]
        }
      }
    ]
  }
}

A babel plugin would be handy for:

import { html, url } from './foo.md'
// ↓↓↓
import { html, url } from '!multi-format-loader?format=html,url!./foo.md'

devongovett commented 6 years ago

Perhaps we want to be explicit about importing a URL with some explicit function to do that, like CSS has a url() function. I'm thinking require.url('./some.png') maybe?

For importing raw files as strings, Parcel shims the node fs.readFileSync method. This could easily be done with a babel plugin as well - I could extract parcel's code if needed.

Generally there are 3 cases:

require('./file') or import './file': should continue to work exactly the same way it does today. You get a JS representation of the file as produced by a specific loader, with a fallback to a URL.
require.url('./file') explicitly imports a URL to the file, even for cases where a loader is registered for that format.
fs.readFileSync explicitly loads the file as a raw string or buffer.

devongovett commented 6 years ago

also, querystrings are not a good interface for this: question marks are a valid character in filenames. This kind of string require filename overloading has never been a good idea IMO.

sompylasar commented 6 years ago

@devongovett There are more valid cases than the ones covered in your comment: 1) exports; 2) url; 3) raw file. Examples were presented above: 4) a React component for an SVG image; 5) HTML markup for Markdown; 6) React component for Markdown; etc.

dmitriid commented 6 years ago

also, querystrings are not a good interface for this: question marks are a valid character in filenames.

However, neither is this:

import { html, url } from '!multi-format-loader?format=html,url!./foo.md'

This is basically a form of a templating/scripting language that for some reason stuck with us against any common sense :) This way lies madness. Below are just two examples I came across at one point or another during development.

css-loader?module&localIdentName=[path][name]--[local]--[hash:base64:5]

style-loader!css-loader?importLoaders=1!autoprefixer-loader

This is a poorly (if at all) specified way of declaring a chain of loaders (build tools) inside a string in source code. Build tool configuration should be left out of source code as much as possible. import {url} from 'style.css' is already enough magic :)

Also, these characters are also valid in filenames: !,-&

gregberge commented 6 years ago

@egoist your idea is good but it requires to re-implement the rules resolution. And it is not that simple since Webpack makes extension optional. I think we should not re-implement the rules algorithm into a loader.

Actually Webpack loaders are applied sequentially, taking this example:

module.exports = {
  module: {
    rules: [
      {
        test: /\.svg$/,
        use: [
          'babel-loader',
          'svgr/webpack',
          'url-loader',
        ],
      },
    ]
  }
}

url-loader is applied on svg, the output is the url of SVG : module.exports = "/my.svg"
svgr/webpack receives the previous output, the original file path and the chain of loaders

Several problems:

svgr/webpack cannot use the "content", it expects an SVG but url-loader transformed it into JS code
If svgr/webpack read the content of the file (using resourcePath) instead of using content, it will override the previous work achieved by url-loader
The solution is to parse previous content, detect a module.exports and re-inject it under a named export

So actually loaders have to know other loaders and make it compatible with them.

In an ideal world, Webpack should handle this, it should have control on it and give us the choice to chain loaders in the way we decide:

module.exports = {
  module: {
    rules: [
      {
        test: /\.svg$/,
        useExports: {
          default: ['babel-loader', 'svgr/webpack'],
          url: ['url-loader'],
        },
      },
    ]
  }
}

It will result in:

import SvgComponent, { url } from './my-file.svg'

We can achieve it by creating a custom loader but it will do its own loaders resolution (a part of Webpack). But if we have Webpack team working on it, it could be a good start before adding it in the core of Webpack.

What do you think?

gregberge commented 6 years ago

About zero configuration, it is another problem for me, you have to decide a common config. If we have a simple way to make it works with all existing loaders, choosing a common config is not a big deal.

papandreou commented 6 years ago

We've been exploring this in AssetGraph for some time, starting way before imports ever arrived. It's an opinionated stack that works without any dev server, and obviously we've also needed a way to express this kind of dependency so the production builder can find the target file, add a hash to the file name, etc.

Currently we support this syntax: '/my-file.svg'.toString('url'). AssetGraph treats that as a first class dependency on /my-file.svg. Syntactically it's not that pretty, but it works without any dev-time bundling (String#toString ignores the argument and just returns /my-file.svg), and it survives uglification.

As someone who also develops tools that process the output of webpack et al., I'm very much in favor of representations that aren't necessarily eliminated by those bundlers, and .toString('url') works very well in that regard.

I get that approaches that don't involve import are already sort of descoped, so just consider this a piece of background info :)

bmeck commented 6 years ago

@devongovett Node and Browsers use URLs for ESM, Query Strings are not part of the file path. Use percent encoding as needed on all file:// scheme paths that contain special characters. In addition the Node fs APIs also accept file:// URL objects for paths.

stereobooster commented 6 years ago

It would be easier to maintain type signatures with named exports. You can do something like this

type image = {
    raw: string,
    url: string,
    component: React.Node
}

For query string based imports you will need to override type manually or use any.

bmeck commented 6 years ago

@stereobooster why? Query String has a different URL and can compose any type of Module Namespace shape it wants.

stereobooster commented 6 years ago

@bmeck I'm talking about type signatures like TypeScript or Flow. I know how to create type signatures based on extension of a file. Not sure if this is possible with query params. Maybe.

FWeinb commented 6 years ago

I had some time and did some experimenting and build a proof of concept babel plugin. It's not published and very much WIP but I liked the idea and needed to try how far this could be pushed.

Some random thoughts I had while working on this:

Configuration of the loaders in the webpack.config will be tedious
Some loaders will probably need some special options passed to work
Maintaining a list of valid file extensions here is not optimal.
I don't know if the proposed syntax is as intuitive as a I previously though

michael-ciniawsky commented 6 years ago

loader.js

const path = require('path')
const { getOptions, interpolateName } = require('loader-utils')

module.exports = function loader (src, map, meta) {
  const options = Object.assign({}, getOptions(this))

  const ctx = this.rootContext || this.context // (webpack 4 || webpack 3)
  const url = interpolateName(this, '[hash].[ext]', {
    content: src,
    context: ctx
  })
  const file = path.relative(ctx, this.resourcePath)

  // meta is/can be set by a previous loader
  const isAsset = options.isAsset || meta.isAsset || false;
  const transform = options.transform || false;

  if (isAsset) {
    // Raw {Buffer} (Loader Mode)
    module.exports.raw = true;

    this.emitFile(...)
  }

  if (transform) {
    src = transfrom(src)
  }

  const result = [
    `export const url = __webpack_public_path__ + '${url}'`,
    `export const file = '${file}'`,
    '',
    isAsset ? `export default \`${src}\`` : false
  ].filter(Boolean).join('\n')

  const cb = this.async()

  cb(null, result)

  return null
}

webpack.config,js

[
  {
    test: /\.txt/
    use: [  'loader' ]
  },
  {
     test: /\.md/
     use: [  
       { 
         loader: 'loader'
         options: {
            transfrom (src) {
              return markdown.render(src)
            }
         }
      }
     ]
   },
   {
     test: /\.svg/
     use: [  
       {
         loader: 'loader', 
         options {
           isAsset: true 
         } 
       },
       'svgo-loader' 
     ]
  }
]

import raw from './file.ext' // Content (raw-loader/url-loader)
import { url, file } from './file.ext' // Asset (file-loader)
import raw, { url, file } from './file.ext' // (both)

sompylasar commented 6 years ago

@michael-ciniawsky :+1: but the version of the implementation I'm reading now (I see you editing, maybe move to a gist with version control?) doesn't let you export additional types, e.g. a React component. Also, `export default \`${src}\`` is unsafe to generate, src may contain string interpolation sequences ${...} which will be interpreted.

gaearon commented 6 years ago

Related: we bumped into a problem implementing a limited subset of this

import url, { ReactComponent } from './file.svg';

The problem is that it breaks importing CSS from SVG, and even if we plug the hole by also exporting toString, an unused component is still not being treeshaken because the CSS/asset pipeline still relies on CommonJS.

If you're interesting in solving this problem, please chime in: https://github.com/facebookincubator/create-react-app/issues/3856.

bmeck commented 6 years ago

@gaearon is https://github.com/indutny/webpack-common-shake unable to shake your CommonJS?

gaearon commented 6 years ago

Interesting, maybe it can. We could consider this as another option.

rgilling commented 6 years ago

Found this issue trying to isomorphically load inline md files in a react app because GA was complaining about loading the resources through isomorphic fetch. Thought I'd share my 2c on it: I got it working for my use case by using babel-plugin-static-fs which converts the node fs calls in the babel pipeline. This worked well on both the SSR server which doesn't have webpack, but is using babel-register (because I haven't managed to get ignore-styles to work with babel-cli), and the client which is using non-ejected CRA.

lifeiscontent commented 6 years ago

@gaearon what if you just returned the urls? and then used babel macros for all the things?

turns out there's even a macro for svgr. https://github.com/evenchange4/svgr.macro. I would imagine we could do the same for markdown and others alike. e.g. https://github.com/pveyes/raw.macro

iansu commented 6 years ago

We considered using babel macros to implement this feature. However, the SVGs end up inlined in the code so if you include the same SVG more than once it will be duplicated, resulting in an unnecessarily large bundle. You can see the discussion here: https://github.com/facebook/create-react-app/issues/3856#issuecomment-358762155

lifeiscontent commented 6 years ago

@iansu thanks for the post, makes sense. 👍

ndelangen commented 6 years ago

@iansu that should pretty much be gone after compression / gzipping though?

facebook / create-react-app