handle missing exports in chunked mode - #1986

This is a bugfix and could be merged at any time. Any thoughts?

I do have strong concerns here:

1. We're allowing imports of missing export bindings to become "orphaned" and magically turn into globals. This is highly inconsistent and I'm not sure when that would ever be valid behaviour. I know we've discussed this before, but I still don't understand how this can make things work out correctly in your codebase.
2. Going down this road means all logic in Rollup needs to accommodate that imported bindings might not resolve to exported variables. This may invalidate a whole bunch of analysis assumptions in various places and create inconsistencies. These changes may just be the start of further analysis code that could hit these same error code paths.

If we can come up with a consistent model here, then great, but I feel like (1) is a deep inconsistency here that makes this use case a hack from the get-go. And having (2) adjust to it further just seems to be introducing algorithmic compromises on footing that isn't consistent to begin with.

Again, as I've said before, I still don't understand how removing interfaces on export boundaries can ever make something work in the first place. Previous examples you've shown me like DockYard/ember-composable-helpers#287 seem like they break public API assumptions?

@guybedford I cleaned up the noisy diff, and made a code tweak. This now makes it more safe, and essentially preserves the original code (and imports that go nowhere). Now this PR no longer generates modules with broken code. As to why this is necessary for the Ember ecosystem, see here.

After experimenting with this some more, I think this only works/makes sense with the preserveModules option. Otherwise, you're right, the bundling of code means that it will have to be broken.

@kellyselden are you saying it works with the preserveModules option due to there being unused export paths?

How would the following subset of behaviour sound to you: When an import is made with a binding that is otherwise treeshaken and unused, we can permit ignoring if it doesnt resolve to a valid existing export in this way.

That's as far as I'd like to go on this at all, and I think could avoid the paths here if the algorithms are written correctly to only do necessary work.

I think we are on the same page, but let me bring in another example.

Guarded usage: https://github.com/samselikoff/ember-cli-mirage/blob/v0.4.2/app/initializers/ember-cli-mirage.js#L21-L23
Import: https://github.com/samselikoff/ember-cli-mirage/blob/v0.4.2/app/initializers/ember-cli-mirage.js#L2
Export: https://github.com/samselikoff/ember-cli-mirage/blob/v0.4.2/blueprints/ember-cli-mirage/files/__root__/config.js

Since

import { missing } from './dep';

if (missing) {
    missing(); 
}

Gets compiled per module in Ember to:

define(['./dep.js'], function (__dep_js) {
    if (__dep_js.missing) {
        __dep_js.missing(); 
    }
});

The code just works.

With this PR, we are currently generating:

if (missing) {
    missing(); 
}

What I would like is to figure out a way to essentially leave it in it's broken state so that we output:

import { missing } from './dep';

if (missing) {
    missing(); 
}

Note: this only makes sense as a compatability feature with preserveModules on, because there is no way around this issue if we are bundling, as you pointed out earlier.

@kellyselden the fact that it "just works" is a consequence of the output format supporting access due to it being on exports. The same code breaks for System and ES module output formats though, so the approach really isn't compatible with Rollup.

@guybedford I know it's a crappy situation to be in, but I'm trying to come up with a solution so we can use rollup in Ember. What about if you have preserveModules enabled and new option, let's call it allowBrokenExports, enabled, then we preserve the code the way it was? It should be noted that this would be a temporary solution, we would be printing a deprecation to allow ember addons to clean up there code, then turn it off/remove the feature from rollup.

One solution here might be to call it unresolvedImports: true and support defining those imports as undefined.

So that:

import { x } from './asdf.js';
console.log(x);

where ./asdf.js contains:

export var y = 'y';

would effectively treat ./asdf.js as if it contains:

export var x = undefined;

export var y = 'y';

So setting up the variable to be defined during the tracing process itself, and managing the piping to "shim" the missing export.

That way we wouldn't need to do existence checks on the vars in all the code, as the tracing code would reliably create the variables as needed.

Then we get the treeshaking benefits etc falling out as well potentially.

@lukastaegert would be interesting to hear your thoughts here too.

One solution here might be to call it unresolvedImports: true and support defining those imports as undefined.

From the various ideas floating around I like this one the best. I really do not like adding all those "does the variable exist?" checks; adding a "virtual" variable to the module each time a missing export is accessed sounds like a great idea. Rendering the variables itself could be handled in the finalisers (e.g. for CJS, no variables need to be added and for ESM, we follow your proposal of exporting undefineds).

@guybedford That is a really good idea! Thank you for being open to the bad situation I am in.

Do you have any guidance on how I would inject code into a module when I've already started the marking phase?

@kellyselden a rough outline I'd suggest would be something like:

1. Make it impossible for the module.traceExport method to return undefined, by inserting a return module.shimMissingExport(exportName) call or similar at the fallthrough points of the method at https://github.com/rollup/rollup/blob/master/src/Module.ts#L694 and https://github.com/rollup/rollup/blob/master/src/Module.ts#L701.
2. The shimMissingExport method (or similar) could then populate this.exports[name] with a fake ExportDescription as well as creating an entry in this.scope.variables to a new custom Variable instance. It would be worth checking the tree shaking works with this ok.
3. If all goes well, this should result in things working, in which case all that should still be needed is to possibly add some extra code to the finalisers to ensure that something is output for these fake exports. Possibly just an extra boolean property to the exports on finalisers indicating the export is fake or shimmed so that it can be at least created with a fake variable.

Perhaps we could support the handleMissingExport hook returning true or false to determine whether to create a variable or to throw? @lukastaegert what are your thoughts on the defaults for this?

@guybedford Sounds like a reasonable way to go about this.

It would be worth checking the tree shaking works with this ok

I would expect the fake variables to receive an .included=true flag just like real variables but yes, this definitely needs checking (and possibly fixing). Then we should be able to use these flags in the finalisers to decide if the fake exports actually need to be generated.

We should also check if there can be any deconflicting issues.

@kellyselden how're things getting on there? Let us know if we can help at all further.

@guybedford I had to take a step back for a bit, but I will pick this up again soon.

I'm interested what's the current status of this pull request :)

Can this be closed now?