Rollup: Explicitly set output.exports

[stephan281094]

Currently, when running a freshly installed Sapper project with Rollup, the following message is displayed:

Entry module "rollup.config.js" is implicitly using "default" export mode, which means for CommonJS output that its default export is assigned to "module.exports". For many tools, such CommonJS output will not be interchangeable with the original ES module. If this is intended, explicitly set "output.exports" to either "auto" or "default", otherwise you might want to consider changing the signature of "rollup.config.js" to use named exports only.

This pull request explicitly sets output.exports to get rid of the above message.

[benmccann]

I'm curious. I don't think I've seen this. Do you know if this only happens with certain version of Rollup or its plugins?

[stephan281094]

All I did was run npx degit "sveltejs/sapper-template#rollup" project-name, then install dependencies and run yarn dev or npm run dev. It should have installed the latest version of everything (based on package.json, of course). Currently these versions are:

@rollup/plugin-babel: 5.1.0
@rollup/plugin-commonjs: 12.0.0
@rollup/plugin-node-resolve: 8.4.0
@rollup/plugin-replace: 2.3.3
rollup": 2.22.1
rollup-plugin-svelte: 5.2.3
rollup-plugin-terser: 5.3.0

[benmccann]

I think this revealed another bug in Sapper's Rollup handling. The warning is supposed to include a link to https://rollupjs.org/guide/en/#outputexports that is not present. Sapper seems to not be printing the url field in the warning when using Rollup to bundle the Rollup config file

[benmccann]

While this works, it might be nicer to maintain consistency with Rollup by doing something more like this:

		const {
			output: [{ code }]
		} = await bundle.generate({
			exports: 'named',
			format: 'cjs'
		});

		// temporarily override require
		const defaultLoader = require.extensions['.js'];
		require.extensions['.js'] = (module, filename) => {
			if (filename === input) {
				module._compile(code, filename);
			} else {
				defaultLoader(module, filename);
			}
		};

		const config = require(input).default;
		delete require.cache[input];

		return config;

See https://github.com/rollup/rollup/blob/21cc03d6b7cf797d948e78b35f82a190e1104bfd/cli/run/loadConfigFile.ts#L78

[stephan281094]

@benmccann Good call, I've updated the pull request with your suggestion.

[benmccann]

thanks! lgtm

[Conduitry]

Is this now making a new assumption about what version of Rollup you're using? E.g., does this now only work with Rollup >=1? If so, this would be a breaking change.

[benmccann]

Good thing to check. This still works with Rollup 1 though. I just tested against the realworld sample app rewound back a few commits to when it was using Rollup 1.15.1

[Conduitry]

I think I would consider this a breaking change though if this now only works with Rollup 1+. I don't know that the required peer dependency is officially documented anywhere - but this code was specifically here to support both < 1 and >= 1 (added by me in #541).

This seems like a reasonable breaking change to make, however, as Rollup 1 is now relatively old. If so, this should probably be lumped together with some other less-jarring but still-breaking changes.

[benmccann]

Oh, I missed that you said >= 1 and just assumed you meant > 1 since 0.x is so old now

So I went back to sveltejs/realworld@0c1c672 and ran it as-is without this change and it worked, but it was relying on an old version of Sapper. Then I upgraded it to the latest version of Sapper (still without this change) and got Error: Cannot find module 'svelte/compiler', so I think we may have broken Rollup 0.x support awhile ago

In either case, I'd be a proponent of merging the other breaking changes that are pending and making the next release 0.28.0