Refactor and simplify umd wrapper

[lukastaegert]

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:
Closes #2274,
closes #1964

Description

This PR refactors the UMD header to achieve several things:

• instead of just using this to find the global variable, which does not work in strict mode, it will first check if self is present and can be used. self is similar to window but should work properly in more contexts.
• the header uses no unnecessary parentheses and becomes thus a little smaller and a little more readable
• in case there are no exports and the noConflict option is not used, there is no difference between the CJS and IIFE intro. Thus we skip the check if we are in a CJS environment. The difference will be that if we are in a CJS environment that is also an AMD environment, AMD will not receive preference. I hope this is not an issue for anyone.
• in case there are no exports and the noConflict option is not used, the global variable is not used and is now removed entirely from the wrapper

Some examples:

// previous wrapper with default export
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	(global.myBundle = factory());
}(this, (function () { 'use strict';

	var main = 42;

	return main;

})));

// new wrapper with default export
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	global.myBundle = factory();
}(typeof self !== 'undefined' ? self : this, function () { 'use strict';

	var main = 42;

	return main;

}));

// previous wrapper without exports
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	(factory());
}(this, (function () { 'use strict';

	globalVar = 1;

})));

// new wrapper without exports
(function (factory) {
	typeof define === 'function' && define.amd ? define(factory) :
	factory();
}(function () { 'use strict';

	globalVar = 1;

}));

I hope I did not miss anything here. Just look at the adjusted tests for more examples. I also removed an unnecessary pair of parentheses from the IIFE wrapper.

[lukastaegert]

@doug @DanielRuf This is a follow-up to your PRs, please see if this solves your issue. I did not adjust the IIFE wrapper as this would have become much more complex when accounting for a different this value but would rather recommend people use the UMD wrapper in such a situation.

[shellscape]

food for thought on readability: I've often used a single template literal enclosure (and line breaks within them) to make the contents more readable in code, and then stripped newlines afterwards. that is of course, a stylistic preference.

[lukastaegert]

Definitely a good idea though I would feel bad about doing something at runtime that can be done at compile time. Then again, I am not 100% satisfied with the code quality in these wrappers but could not muster the energy and time to do a larger refactoring :( I.e. if we extract more and meaningful sub-strings, the template strings would not get that complicated and the outermost string would just concatenate a few abstract building blocks.

[lukastaegert]

Thanks for taking a look!

[shellscape]

👍

[guybedford]

👍 thanks for picking this one up @lukastaegert.

[thysultan]

instead of just using this to find the global variable, which does not work in strict mode...

Since there are cases when self could be defined but not point to the global object would the following also work this || self?

[lukastaegert]

Hmm, actually a good point. Since with the latest refactoring, the global object is only retrieved when it is actually needed and therefore everything will fail if both this and self are undefined, your version is better and even shorter. Will consider making this change for the 1.0 release.

[guybedford]

In Node.js this is the exports value, so that can't work unfortunately... all other forms become a triple branch, but there's nothing wrong with that too.

[thysultan]

@guybedford Since node doesn't define self as a global object by default. The current(or previous) umd wrapper has worked so far for node so isn't this what you'd want?

[guybedford]

I guess global isn't used in the CJS path so it was never an issue, but any place it is used in future from the wrapper would expose the bug.

[lukastaegert]

Explicit access to the global object is only and will only ever be necessary for the IIFE path but we still need to pass in this to make it available inside the wrapper. Therefore it is not a bug but a necessity with the misleading name global where it should rather be something like globalThis. A real issue I see after some consideration is that using this || self would fail wrongly in a scenario where this is undefined, self is not defined, but there is an AMD loader present.

How about changing the wrapper like this and only testing for self in the IIFE case inline? This would also slightly improve performance in the other cases.

// default export
(function (globalThis, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	(globalThis || self).myBundle = factory();
}(this, function () {
// body
}));

// named exports
(function (globalThis, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
	typeof define === 'function' && define.amd ? define(['exports'], factory) :
	factory((globalThis || self).myBundle = {});
}(this, function (exports) {
// body
}));

[guybedford]

Sounds good, and if there are any global issues we can always fix them later.

[simonbrunel]

I might have missed something in the release notes but it seems that global is not anymore injected in UMD builds in case there is no default export but external dependencies. For example, with the following rollup (v0.68.0) config:

{
	input: 'src/index.js',
	output: {
		file: `dist/${pkg.name}.js`,
		banner: banner,
		format: 'umd',
		globals: {
			'chart.js': 'Chart'
		}
	},
	external: [
		'chart.js'
	]
}

I'm getting this wrapper:

(function (factory) {
	typeof define === 'function' && define.amd ? define(['chart.js'], factory) :
	factory(global.Chart);
}(function (Chart) { 'use strict';
...

with this error:

Uncaught ReferenceError: global is not defined 

and in case of CJS loader, shouldn't we expect factory(require('chart.js'))?

[lukastaegert]

Yes, it seems I missed something here. I hope I have the fix together tomorrow, I am currently working on better tests so this does not happen again.

[lukastaegert]

@simonbrunel Fix at #2600

[simonbrunel]

Thanks a lot @lukastaegert

[eleven-net-cn]

@lukastaegert @simonbrunel

How do I output the following example code?

// new wrapper with default export
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
	typeof define === 'function' && define.amd ? define(factory) :
	global.myBundle = factory();
}(typeof self !== 'undefined' ? self : this, function () { 'use strict';

	var main = 42;

	return main;

}));

I need to get self first, so need to inject the following code

typeof self !== 'undefined' ? self : this

We can use Webpack in the following ways

module.exports = {
  // ...
  output: {
    library: 'myLib',
    libraryTarget: 'umd',
    filename: 'myLib.js',
    // to replace global this
    globalObject: `typeof self !== 'undefined' ? self : this`,
  },
};