handle missing exports in chunked mode - #2118

The plan in going through with this approach was to be able to avoid having to have special cases like this - otherwise all logic around exports needs to be considered against this case which may lead to bugs.

I really do think we should ensure the shimmed export otherwise behaves exactly like any other, with module being defined here.

Ok I'll take another stab at it.

Thanks, will be a great help to ensure the assumptions are minimal.

Rather than have the missingExport hook provide this functionality, how about making this a flag option - shimMissingExports: true?

That is already the way to stop the default behavior of hard erroring (in non-code-splitting mode). Are you suggesting checking here for shimMissingExports and conditionally not erroring?

Yes exactly, so that this can be done without a plugin.

done

Would it be possible for ShimVariable to inject itself into originals here on the module itself so that variable lookups play out correctly? Or does that introduce too many cross-cutting issues?

I thought about it, but I couldn't figure out how to get access to the NamespaceVariable.originals from earlier in the traceExport call stack at this point of shimming.

I think NamespaceVariables are created after the initial tracing in module right? So that exports should already be shimmed by then and then the getExports() call from the namespace constructor should include the shimmed names?

Let me know if there are any other issues - I know it's a bit to investigate.

Let me know if you have further questions on this one.

I was able to remove the NamespaceVariable.originals workaround completely. Something I changed must have made it obsolete.

Could we make this a default flag over an object rather?

I figured module.traceExport(name, { isExportAllSearch: true }); would be more clear to the reader than, module.traceExport(name, true);, but I can change it if you want.

That would be great just to avoid the object allocation, for what it's worth. It doesn't seem like the flags will be extending anytime soon.

done

I wonder if we could shift these types of logic out of the finalisers by rendering a line like:

export const $$shim = null;

as part of the core rendering process, then setting the specifier.local value to $$shim.

I'm not sure I follow. Wouldn't it get really complicated if there were multiple missing exports for a given modules? Like:

import { missing1, missing2 } from './foo';

missing1('bar');
missing2('baz');

Sorry I should have clarified rendering just a ${varOrConst} $$shim = null;, so that in foo itself the output would be:

const $$shim = null;

export { $$shim as missing1, $$shim as missing2 }

Let me know if that makes sense?

It does now. But aren't you worried about adding a var after the deconflicting step? Any name we choose, like $$shim, would possibly conflict with an existing $$shim var.

I made the change, but the conflict issue still worries me.

$$shim can be added to the list of names to deshadow in the setIdentifierResolutions function of Chunk to ensure proper handling of this.

I'm now running into the problem where my output is

const $$shim$1 = null;

var $$shim = null;
export { $$shim$1 as $$shim, $$shim };

This is a dependency and an entry point. My existing unused $$shim was renamed to $$shim$1 because of the deshadowing, but is still exported as $$shim because of entry point. Now this conflicts with my shimmed export $$shim. It's almost like we need to reverse the renaming by leaving existing $$shims as is, and instead do something like chunk.generateUniqueVarAndExportName() for the shimmed export name. Am I on the right track?

I got something working.

This can be done in the preRender of Chunk.ts.

Also the const should be a ${varOrConst} reference.

The preRender looks a little foreign to me. Would you think it goes in prepend, append, or something else? How do I get the var before the exports block, but after any module wrappers so that it's not in global scope?

Just like at

preRender renders the main body source, while render renders the finalizers. I think preRender would be a good place. Note that there is a hoistedSource for bindings that must be available early this could be added to at

If I do this, it works for everything except systemjs, where the output is

System.register([], function (exports, module) {
	'use strict';
	return {
		execute: function () {

			exports('$$shim$2', $$shim$2);
			var $$shim$2 = null;

			const $$shim = exports('$$shim$1', null);

		}
	};
});

I may have the change the order of functionExports in systemjs to get it to work this way.

made the change.

This test is a little hard to debug as it tries to do everything in one test while at the same time forgetting some important combinatorics that actually fail. I would suggest to split this up into several more focused tests.

• A golden path test showcasing how this feature actually works without name conflicts (helpful for documentation purposes). I think a single import file should be enough here 😉
• One or even more separate test(s) focusing explicitly on name conflicts. The most important situation that should be covered is that output produced by this feature is feeded back into rollup but there is more. What is actually not working correctly and needs to be fixed is the following (which pretends to be a set of shimmed exports but apparently someone has edited it manually 😜):

// main.js
import { missing, _shimmedExport, _shimmedExport$1 } from './dep.js';
_shimmedExport() // note that we are actually *using* some of the shimmed exports
_shimmedExport$1()
missing();

// dep.js
// added a function with a side-effect here
const _shimmedExport = () => console.log('effect');
export { _shimmedExport, _shimmedExport as _shimmedExport$1 };

// output - main.js
import { _shimmedExport as _shimmedExport$1 } from './d1.js';

_shimmedExport$1();
_shimmedExport$1();
_shimmedExport(); // now hello, who do you belong to!? Isn't this the missing one?

// output - dep.js
const _shimmedExport$1 = () => console.log('effect');

// so apparently there *are* several exports here but they are all wired up the same
export { _shimmedExport$1 as _shimmedExport, _shimmedExport$1, _shimmedExport$1 as missing };

If the combinatorics are checked for the non-compact case, a single compact mode test should suffice, though.

Instead of searching we might just use this.graph.exportShimVariable here which would also be a little clearer.

Perhaps clarify Missing export here.

done

missingExportName or exportName perhaps?

done