Skip to content
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

Merged
merged 3 commits into from Oct 27, 2019

Conversation

bterrier
Copy link
Contributor

@bterrier bterrier commented Oct 21, 2019

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:
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 the return from the bundle entry point, making it possible to retrieve this value.

@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #3181 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
src/finalisers/iife.ts 94.59% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4c8b43...da97c76. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

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.

@eight04
Copy link

eight04 commented Oct 23, 2019

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;'
  }
}

@bterrier
Copy link
Contributor Author

bterrier commented Oct 23, 2019

@eight04 The footer solution is not good. It creates a variable name outside of the IIFE scope.

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.

@lukastaegert
Copy link
Member

lukastaegert commented Oct 23, 2019

But what do you need the return statement for? It does not have any effect.

@bterrier
Copy link
Contributor Author

bterrier commented Oct 23, 2019

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 var name = pollutes the scope and prevent getting back the return statement as var name = evals to undefined.

@lukastaegert
Copy link
Member

lukastaegert commented Oct 23, 2019

I see. So how about keeping your changes and adding back the error in form of a warning instead?

@bterrier
Copy link
Contributor Author

bterrier commented Oct 24, 2019

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?

@lukastaegert
Copy link
Member

lukastaegert commented Oct 24, 2019

How about If you do not supply "output.name", you may not be able to access the exports of an IIFE bundle.

lukastaegert and others added 2 commits Oct 27, 2019
Also migrate UMD test and remove some unused folders
Copy link
Member

@lukastaegert lukastaegert left a comment

Thanks! I added another test for the warning to fix coverage, otherwise this can be merged.

@lukastaegert lukastaegert merged commit 2f7e064 into rollup:master Oct 27, 2019
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants