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
Remove the need to provide an output name for IIFE bundles #3181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3181 +/- ##
=======================================
Coverage 90.19% 90.19%
=======================================
Files 167 167
Lines 5907 5907
Branches 1797 1797
=======================================
Hits 5328 5328
Misses 350 350
Partials 229 229
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation here but I must say I am not happy about the solution for two reasons.
The lesser reason is that this is not only an issue for IIFE but also for UMD bundles, so this creates a disparity between the two formats.
The main reason is that for most people when they export bindings from their entry point, the error was intended as a helpful message what option they need to make the export available to the outside. So as a compromise, I would be ok with your solution if instead of an error, we at least display a warning that no name was provided and exports will therefore not be available. Then it would be usable for you, and you could just filter the warning in an onwarn
handler.
As for the core issue in rollup/rollup-plugin-commonjs#274, I believe a much better solution without any overhead is the first suggestion here rollup/rollup-plugin-commonjs#274 (comment)
Basically you create a new entry point for your code that contains nothing but an import of your old entry point without any bindings, and bundle that one:
// a-require.js
const b = require('./b');
console.log(b.foo);
// b.js
module.exports = {foo: 'bar'};
// entry.js, this is the wrapper entry point
import './a-require';
Now if you bundle entry.js
(and do not forget the commonjs plugin), you will get this output:
(function () {
'use strict';
var b = {foo: 'bar'};
console.log(b.foo);
}());
This will work, no matter if your original entry point is CJS or ESM. In case your original entry points DOES have exports, this has an additional advantage as tree-shaking will try to remove all code that does not have observable side-effects such as calling or modifying global variables, so you will get a smaller bundle.
AFAIK, @bterrier 's issue has nothing to do with commonjs (rollup/rollup-plugin-commonjs#274 (comment)). @bterrier 's goal is to generate an IIFE with a return value and without a variable declaration. Though this use case is pretty rare and I'm not really sure whether it should be built into Rollup. BTW, if @bterrier just want to generate a script that the last statement is the module exports, try adding a footer: {
output: {
footer: 'name;'
}
} |
@eight04 The footer solution is not good. It creates a variable To make it would I would need to define both a header and footer to wrap it in an extra IIFE so that it generates : (function () {
<bundle>
return name;
}()); This adds noise to the rollup command (or config) and adds an extra useless function call in JS. I reckon that my use case may be pretty rare, but allowing IIFE exports without name is not a feature that has to be built into rollup. On the contrary, it is the forbidding of nameless IIFE that is a "feature" that was added to rollup. This is backed up by the fact that my commit is mostly removing code. |
But what do you need the return statement for? It does not have any effect. |
Yes, it does. When you run JS code in a JS engine (from command line or from another language) you get the result of the last statement. Having the |
I see. So how about keeping your changes and adding back the error in form of a warning instead? |
Fine with me. Is there any specific text I should put in the warning? |
How about |
f9e2d0a
to
9a1d5c1
Compare
9a1d5c1
to
f81085a
Compare
Also migrate UMD test and remove some unused folders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added another test for the warning to fix coverage, otherwise this can be merged.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
rollup/rollup-plugin-commonjs#274
Description
This remove the requirement to provide a name (
--name
or--output.name
) when building an IIFE bundle.The previous behavior was to force the user to provide a name and add
var name =
in front of the IIFE.Practically it means that when running the bundle the last evaluated statement is the
var name =
assignation which "returns"undefined
.Now, when no name is provided, instead of failing, the bundle is generated without
var name =
. Which means that when running the bundle, the JS environment is not polluted by an extra variable and the last evaluated statement is thereturn
from the bundle entry point, making it possible to retrieve this value.