Provide es module template

[Andarist]

Currently the loader is outputting cjs format, we should support both cjs and es2015 modules - probably based on the webpack's + babel versions used.

Why this is important?
This prevents webpack3 from scope hoisting generated components.

If we settle how this should be solved I can offer my help with the implementation.

[jhamlet]

Can you provide some reference links to the items you have mentioned (ES2015 modules, webpack 3 scope hoisting, etc...)?

This way we are (ahem) on the same page regarding details.

[Andarist]

Sure, gonna do that later today. Thanks for the answer.

[Andarist]

@jhamlet
Ok, so basically this plugin emits js modules in commonjs format (using modules.exports and require), problem with those is that they are not easily statically analyzable (unlike es2015 modules - so they prevent various optimizations:

• tree-shaking - technique popularized by rollup (now also included in webpack2+), basically by using es2015 syntax its possibly to know at build time which exports were not used by the consumer and thus its possible to strip them down from the final bundle
• scope hoisting - another technique popularized by rollup ;) webpack originally wraps each module it imports in a closure, and such closure needs to be evaluated in runtime to create a module (it costs startup time), so basically if possible (with correct configuration) a module is not wrapped in a closure but rather hoisted to the single closure with other modules and each name conflict is resolved by suffixing or something similar

[jhamlet]

@Andarist What needs to be done in the output string to make it compatible with es2015 modules?

[Andarist]

@jhamlet basically such change should be done imho

module.exports = R.curry(function stringify (opts, tree) {
    var displayName = opts.displayName;

    var preamble = [
-       'var React = require(\'react\');',
+       'import React from \'react\';',
        '',
-       'function ' + displayName + ' (props) {',
+       'export default function ' + displayName + ' (props) {',
    ];

    var postamble = [
        '}',
        '',
-       displayName + '.displayName = ' + JSON.stringify(displayName) + ';',
-       '',
        displayName + '.defaultProps = ' + JSON.stringify(tree.props || {}) + ';',
-       '',
-       'module.exports = ' + displayName + ';',
-       '',
-       displayName + '.default = ' + displayName + ';',
        ''
    ];

    return preamble.
        concat([fromObject(tree, true)]).
        concat(postamble).
        join('\n');
});

but probably which version of the module is emitted should be based on a configuration or deduced from other plugin's config - babel-loader i.e. to see if its configured to transpile es modules or not, aint sure if this is possible though, im not familiar with webpack's plugin architecture etc

If that is not possible a simple option in the svg-react-loader like options.modules: true should suffice and I believe its a better option to make true as a default, but thats ur choice :)

[jhamlet]

This doesn't seem to provide much bang for the amount of work that would have to be done adding options, checking babel options, and what-not.

This might be a situation where an additional loader can be used in the pipeline utilizing svg-react-loader's raw ability. Then you could write out whatever module structure you chose.

[Andarist]

The bang here is smaller output in people's bundles, less bytes shipped over the wire is always nice to have in my opinion.

Checking babel's options was just an idea, surely more complex and im not sure if even doable. Second way - just adding a new option should be pretty easy and not time-consuming at all. I can provide a PR for this if you give a green light for the idea.

Using raw is pretty neat idea I must admit, however quite cumbersome when it comes to usage - more setup and people would have to know that there is such a combo of using raw + some kind of other parser of JSON to es module. Also a lot more to write for such a parser than just adding support for a new option here.

Even if you dont feel its much of a bang, I feel its a right direction even if only because of the fact that es modules are part of the JavaScript language specification and they will get more and more adoption over the following years, while commonjs format is not.

[jhamlet]

By bang, I meant that nothing is going to be stripped out with tree shaking since there is only one export in the generated module. And react is basically non-negotiable, so that would never be stripped out.

Regarding scope hoisting (module concatenation) : I still do not see what we are getting there -- this just makes sure you only ever import a module once. In any other code splits, those just use a function to return the module from another split. So, actually you are increasing execution time (fractionally) with that extra function invocation.

One of the goals with the 0.4.x version of svg-react-loader was to do away with transpilation.

I agree it is a minor change to start using import instead of require, however that then requires transpilation until the JavaScript ecosystem catches up.

I would rather wait until import is de rigueur and make the easy change.

[Andarist]

Regarding scope hoisting (module concatenation) : I still do not see what we are getting there -- this just makes sure you only ever import a module once. In any other code splits, those just use a function to return the module from another split. So, actually you are increasing execution time (fractionally) with that extra function invocation.

Actually scope hoisting can be used on its own without code-splitting.

Example output without scope hoisting used:

// module
"./path/to/icon.svg": (function(module, exports, __webpack_require__) {
  "use strict";
  function IconComponent() {}
  module.exports = IconComponent;
 })

// importing module
var __WEBPACK_IMPORTED_MODULE_2_assets_minimize_svg__ = __webpack_require__("./path/to/icon.svg")

Each module - so each svg icon - is wrapped in a closure by webpack and need to be retrieved for usage in runtime from the giant object/array of modules.

Whereas es modules can be scope hoisted and they are literally put in a single scope in form looking similar to this:

function IconComponent$1() {}

[jhamlet]

@Andarist

That second example does make a bit of sense... but, it isn't really buying much. Especially if the module invocation is not cached (it is hard to see what is going on in the IconComponent$1() example).

I am willing to look at anything you come up with, and anything that requires transpilation, or configuration for such, will be suspect (more moving parts, likely leading to future headaches).

I will be far more convinced of changes if you can come up with some samples that demonstrate quantitatively how much can be saved (in bytes, as well as execution time).

NOTE: Not trying to be a pain. I am just not convinced this is all that valuable at this point in time.

;-j

[Jessidhia]

Making your transform produce commonjs also causes extra work on the importing side -- if you import with import, webpack must always pass your import through an adapter to convert the commonjs to an ESM shape at runtime.

No checking at all for babel settings or external config is necessary as webpack supports import and export natively. This could only be a problem at all for webpack 1.x setups.