Fix build for es modules in browser

[YerkoPalma]

This PR should fix the current module build in production. It make two things

1. Remove (actually define globals) process calls (fix Remove usage of process.env.NODE_ENV in ES module build #2907)
2. Replace external module call with url, so module system is happy with it.

I tried locally and it works, also tests keep green, but I'm used to rollup so it would be cool to double check this solution.

[TimvdLippe]

config.paths is always undefined here.

[YerkoPalma]

For commonJS build yes, it is, but it was it before so it is not doing anything new. Anyway, might be clearer to have something like

if (env === 'es') config.paths = { 'symbol-observable': 'https://unpkg.com/symbol-observable?module' }

[timdorr]

@YerkoPalma Can we use that form? It'll be easier to understand for most folks.

[YerkoPalma]

No problem 👍

[YerkoPalma]

@timdorr @TimvdLippe updated according to reviews. Thanks :)

[timdorr]

How does something like Webpack handle these things? The URL and its structure is arbitrary, so it can't know to transform it to a 'symbol-observable' module. Does this work when used in a non-module-aware browser (IE11, for instance)?

[TimvdLippe]

I would say that an ES modules is only supposed to be loaded by browsers which support modules. For IE11 you need to bundle anyway, so you can keep on using the lib/ variant. ES modules should be for modern browsers.

[timdorr]

Yes, that's correct. But Webpack and others are module-aware and can convert these import/export statements into standard ES5 code. I would like to know what they do when presented with a URL like this, as this will be the file most people will use (it's default for Webpack, Rollup, and Parcel, at least).

[YerkoPalma]

This is a transparent change for bundlers that transpile to es5, because es5 use commonJS, and the unpkg url is used only for es modules. About the url, that's the official npm cdn repo, and the ?module querystring use the es module version of the package, so it is not that arbitrary.

And like @TimvdLippe said, old browsers would use commonJS build, so non of this matter to those

Hope this makes sense.

[timdorr]

Unpkg isn't an official product of npm. Michael Jackson built and maintains it. It's solid and reliable, but it's definitely unofficial and 3rd-party to npm.

Recent versions of Webpack and other bundlers will be pulling the ES module version, as it's defined in our package.json and they prefer the module entry point over main. They transpile import statements to ES5 code, but definitely source that code from the module entry point if available. My concern is what happens when they run into a URL for the module name/source. Can we test that somehow?

[YerkoPalma]

Well, about the unpkg cdn, I don't know of any official npm cdn, the only thing I know is that some time ago npm changed npmcdn to unpkg, in fact, if you go to https://npmcdn.com it redirects to unpkg. Anyway, if the redux team has some better cdn I'm glad to use that.

Now, about the bundlers, using a url is part of the es modules spec, using relative local paths is also part of the spec, using names directly is not. So, if those bundlers are using the module version of a package and not supporting the url spec, then they are implementing non-standard behaviour, and I'm not sure what to do about it.

IMHO this PR should stay as is, maybe, as said before, change the cdn for other, but keeping the url as it is regardless of bunflers.

[TimvdLippe]

symbol-observable does expose a es module: https://github.com/benlesh/symbol-observable/blob/master/es/index.js Therefore, I think it is safer to import from there.

[YerkoPalma]

So we should use the raw git version? like https://raw.githubusercontent.com/benlesh/symbol-observable/master/es/index.js

In that case, we would be pointing to master branch which is a development branch. If we use the last release tag version we would have to manually update in every future release. Is that ok?

[TimvdLippe]

Hm, the package should just be there in the node_modules, so you can import from there.

[timdorr]

Well, about the unpkg cdn, I don't know of any official npm cdn, the only thing I know is that some time ago npm changed npmcdn to unpkg, in fact, if you go to https://npmcdn.com it redirects to unpkg.

That was the original name of the project, but Michael changed it to avoid confusion (I believe npm may have requested he change it). There is no official npm cdn for anything other than package tarballs.

[timdorr]

I don't really care what URL we use. I care about how it's interpreted by most bundlers. I don't think a URL for the module source is going to work. But I'd like testing done to confirm that.

[timdorr]

OK, build a copy of this PR locally for testing.

Parcel interprets it as the https package and attempts to auto-install it, which fails:

 ⇶ parcel index.html
Server running at http://localhost:1234
npm ERR! Only absolute URLs are supported

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/timdorr/.npm/_logs/2018-08-13T17_50_33_811Z-debug.log
🚨  /Users/timdorr/forks/redux/es/redux.js:1:25: Failed to install https:.
> 1 | import $$observable from 'https://unpkg.com/symbol-observable?module';

Webpack also can't resolve it:

 ⇶ webpack-cli index.js
Hash: 0e394ac4988b4f310386
Version: webpack 4.16.5
Time: 342ms
Built at: 08/13/2018 1:53:07 PM
 1 asset
Entrypoint main = main.js
[0] ./index.js 41 bytes {0} [built]
[1] ./es/redux.js 28.6 KiB [built]
[2] (webpack)/buildin/global.js 489 bytes [built]

ERROR in ./es/redux.js
Module not found: Error: Can't resolve 'https://unpkg.com/symbol-observable?module' in '/Users/timdorr/forks/redux/es'
 @ ./es/redux.js 1:0-70 507:12-24 522:11-23
 @ ./index.js

So, this won't work as-is.

[Wildhoney]

@timdorr does Redux really need the symbol-observable dependency?

Although I appreciate that symbol-observable is a ponyfill and thus slightly different to a compile-time polyfill, is it not a valid argument that such a detail is more the responsibility of the build process itself? For instance with Webpack's @babel/env core-js will be bundled which would polyfill Symbol.observable appropriately.

It's mildly exasperating that Redux transpiles to a purported ES output that is not compatible with the ES spec, as loading Redux as-is in the browser without any compile step results in:

Uncaught TypeError: Failed to resolve module specifier "symbol-observable". Relative references must start with either "/", "./", or "../".

I do agree with your point though about the proposed change in this PR. Relying on a third-party CDN for resolving a very popular JS library of course has its own problems, such as a handful of security concerns. Yet I think we could resolve it by moving the concern of polyfilling to the build process which most people _{[citation needed]} are doing nowadays anyway.

Thoughts?

[TimvdLippe]

It's mildly exasperating that Redux transpiles to a purported ES output that is not compatible with the ES spec, as loading Redux as-is in the browser without any compile step results in:

I thought more about this and I become more and more convinced that this is not a problem Redux should solve. We had the same issue in Polymer and decided that name paths were the way forward, given the way NPM works. See package-community/discussions#2 for more context as well as the following standards proposal: https://github.com/domenic/package-name-maps

Therefore, I think it is okay (for the timebeing) to simply publish names rather than paths. CDNs like unpkg already take care of this and, in the case of Polymer, tools like polymer serve handle this.

Thus, the only change that this PR should contain is the extra rollup plugin to handle the process variable. Other than that, I think we should keep the current module imports.

[TimvdLippe]

Per #3087 (comment) let's revert this change. We can use unpkg to make sure names are transformed to paths on the fly.

[timdorr]

OK, looking at the output there are two problems:

1. The polyfill completely overwrites the process global, which will break it for any ES module bundler that wants to do minification.
2. It includes a whole lot of other stuff that's unrelated That adds up to about 6KB of extras in there which doesn't appear to be tree-shakable.

I'll include the build output below, but it doesn't appear this plugin will work for us. A similar, more minimal technique may be to "ponyfill" just process.env. As such, I'm going to close this out for now.

es/redux.js

import $$observable from 'symbol-observable';
var global$1 = typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {};
// shim for using process in browser

// based off https://github.com/defunctzombie/node-process/blob/master/browser.js
function defaultSetTimout() {

throw new Error('setTimeout has not been defined');

}

function defaultClearTimeout() {

throw new Error('clearTimeout has not been defined');

}

var cachedSetTimeout = defaultSetTimout;

var cachedClearTimeout = defaultClearTimeout;

if (typeof global$1.setTimeout === 'function') {

cachedSetTimeout = setTimeout;

}

if (typeof global$1.clearTimeout === 'function') {

cachedClearTimeout = clearTimeout;

}
function runTimeout(fun) {

if (cachedSetTimeout === setTimeout) {

//normal enviroments in sane situations

return setTimeout(fun, 0);

}

// if setTimeout wasn't available but was latter defined

if ((cachedSetTimeout === defaultSetTimout || !cachedSetTimeout) && setTimeout) {

cachedSetTimeout = setTimeout;

return setTimeout(fun, 0);

}

try {

// when when somebody has screwed with setTimeout but no I.E. maddness

return cachedSetTimeout(fun, 0);

} catch (e) {

try {

// When we are in I.E. but the script has been evaled so I.E. doesn't trust the global object when called normally

return cachedSetTimeout.call(null, fun, 0);

} catch (e) {

// same as above but when it's a version of I.E. that must have the global object for 'this', hopfully our context correct otherwise it will throw a global error

return cachedSetTimeout.call(this, fun, 0);

}

}

}

function runClearTimeout(marker) {

if (cachedClearTimeout === clearTimeout) {

//normal enviroments in sane situations

return clearTimeout(marker);

}

// if clearTimeout wasn't available but was latter defined

if ((cachedClearTimeout === defaultClearTimeout || !cachedClearTimeout) && clearTimeout) {

cachedClearTimeout = clearTimeout;

return clearTimeout(marker);

}

try {

// when when somebody has screwed with setTimeout but no I.E. maddness

return cachedClearTimeout(marker);

} catch (e) {

try {

// When we are in I.E. but the script has been evaled so I.E. doesn't  trust the global object when called normally

return cachedClearTimeout.call(null, marker);

} catch (e) {

// same as above but when it's a version of I.E. that must have the global object for 'this', hopfully our context correct otherwise it will throw a global error.

// Some versions of I.E. have different rules for clearTimeout vs setTimeout

return cachedClearTimeout.call(this, marker);

}

}

}

var queue = [];

var draining = false;

var currentQueue;

var queueIndex = -1;
function cleanUpNextTick() {

if (!draining || !currentQueue) {

return;

}

draining = false;

if (currentQueue.length) {

queue = currentQueue.concat(queue);

} else {

queueIndex = -1;

}

if (queue.length) {

drainQueue();

}

}
function drainQueue() {

if (draining) {

return;

}

var timeout = runTimeout(cleanUpNextTick);

draining = true;
var len = queue.length;
while (len) {
    currentQueue = queue;
    queue = [];
    while (++queueIndex < len) {
        if (currentQueue) {
            currentQueue[queueIndex].run();
        }
    }
    queueIndex = -1;
    len = queue.length;
}
currentQueue = null;
draining = false;
runClearTimeout(timeout);

}

function nextTick(fun) {

var args = new Array(arguments.length - 1);

if (arguments.length > 1) {

for (var i = 1; i < arguments.length; i++) {

args[i - 1] = arguments[i];

}

}

queue.push(new Item(fun, args));

if (queue.length === 1 && !draining) {

runTimeout(drainQueue);

}

}

// v8 likes predictible objects

function Item(fun, array) {

this.fun = fun;

this.array = array;

}

Item.prototype.run = function () {

this.fun.apply(null, this.array);

};

var title = 'browser';

var platform = 'browser';

var browser = true;

var env = {};

var argv = [];

var version = ''; // empty string to avoid regexp issues

var versions = {};

var release = {};

var config = {};
function noop() {}
var on = noop;

var addListener = noop;

var once = noop;

var off = noop;

var removeListener = noop;

var removeAllListeners = noop;

var emit = noop;
function binding(name) {

throw new Error('process.binding is not supported');

}
function cwd() {

return '/';

}

function chdir(dir) {

throw new Error('process.chdir is not supported');

}function umask() {

return 0;

}
// from https://github.com/kumavis/browser-process-hrtime/blob/master/index.js

var performance = global$1.performance || {};

var performanceNow = performance.now || performance.mozNow || performance.msNow || performance.oNow || performance.webkitNow || function () {

return new Date().getTime();

};
// generate timestamp or delta

// see http://nodejs.org/api/process.html#process_process_hrtime

function hrtime(previousTimestamp) {

var clocktime = performanceNow.call(performance) * 1e-3;

var seconds = Math.floor(clocktime);

var nanoseconds = Math.floor(clocktime % 1 * 1e9);

if (previousTimestamp) {

seconds = seconds - previousTimestamp[0];

nanoseconds = nanoseconds - previousTimestamp[1];

if (nanoseconds < 0) {

seconds--;

nanoseconds += 1e9;

}

}

return [seconds, nanoseconds];

}
var startTime = new Date();

function uptime() {

var currentTime = new Date();

var dif = currentTime - startTime;

return dif / 1000;

}
var process = {

nextTick: nextTick,

title: title,

browser: browser,

env: env,

argv: argv,

version: version,

versions: versions,

on: on,

addListener: addListener,

once: once,

off: off,

removeListener: removeListener,

removeAllListeners: removeAllListeners,

emit: emit,

binding: binding,

cwd: cwd,

chdir: chdir,

umask: umask,

hrtime: hrtime,

platform: platform,

release: release,

config: config,

uptime: uptime

};
/**

These are private action types reserved by Redux.
For any unknown actions, you must return the current state.
If the current state is undefined, you must return the initial state.
Do not reference these action types directly in your code.

*/

...