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.

Sign up for GitHub

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

Jump to bottom

CrossChunkCodeMotion introduces JS runtime error in ES2015+ #3752

Closed
DavidANeil opened this issue Jan 14, 2021 · 6 comments
Closed

CrossChunkCodeMotion introduces JS runtime error in ES2015+ #3752

DavidANeil opened this issue Jan 14, 2021 · 6 comments
Assignees
@brad4d
Labels
triage-done Has been reviewed by someone on triage rotation.

Comments

@DavidANeil
Copy link
Contributor

Given the following code:

// main.js
goog.module('_main');

const LocalConstant = {a: 5};

class Base {
    constructor() {
        console.log(LocalConstant);
    }
}

exports.Base = Base;

console.log(LocalConstant);
// other.js
goog.module('_other');

const main_1 = goog.require('_main');

class Child extends main_1.Base {
    constructor() {
        super();
    }
}

exports.Child = Child;

goog.global.sideeffect = new Child();

Compiling into ES2015

$ java -jar closure-compiler-v20201102.jar --language_out ECMASCRIPT_2015 --emit_use_strict false --compilation_level ADVANCED_OPTIMIZATIONS --formatting PRETTY_PRINT --js_output_file=mainout.js --chunk mainout:2 --chunk otherout:1:mainout ~/github/closure-library/closure/goog/base.js main.js other.js

Produces the following output:
mainout.js:

var a = this || self;
const c = {a:5};
console.log(c);

otherout.js:

var d = class {
  constructor() {
    console.log(c);
  }
};
a.b = new class extends d {
  constructor() {
    super();
  }
};

As you can see, the class Base (d) from main.js has been moved to a different chunk, since it is unused in its own chunk.
Unfortunately the LocalConstant (c) is not available in the context of otherout.js, because it is declared with const in mainout.js.
If main.js is adjusted to include a exports.LocalConstant = LocalConstant; then const c becomes var c and therefore is available in the other module.
Before ES2015 const did not exist, so this worked without error.

There are 2 possible fixes, I am not sure which is preferred:

  1. Change the CrossChunkCodeMotion CompilerPass to not move the Base class out of its chunk, because it is tied to the local variable, and therefore a part of the global cycle created by the side-effect of logging the constant.
  2. Change some other (?which?) CompilerPass to synthesize the exports.LocalConstant = LocalConstant;

I am going to start looking into the first of those as a contribution, mostly just because it seems easier.
@ChadKillingsworth
Copy link
Collaborator

Are you utilizing the --rename_prefix_namespace flag? If not, then yes all cross chunk references will be broken if they aren't in the global scope.

@DavidANeil
Copy link
Contributor Author

That flag hasn't ever been viable because it doesn't create the namespace object before trying to write to it.

Currently working around the runtime error by transforming all module-level const and let to var.

@rishipal rishipal added the triage-done Has been reviewed by someone on triage rotation. label Jan 15, 2021
@ChadKillingsworth
Copy link
Collaborator

You either need to define the namespace externally, or you can use an output wrapper:

--rename_prefix_namespace=ns --output_wrapper="(function(ns) { %OUTPUT% })(window.ns = window.ns || {})"

@concavelenz
Copy link
Contributor

To be clear, this isn't a problem with CCCM but with how the code is loaded. The compiler generates code that expects to be loaded as global scripts, if you want isolation between chunks --rename_prefix_namespace is the correct solution.

I do think this is something that should be clearly document and likely is not.

@concavelenz concavelenz assigned concavelenz and brad4d and unassigned concavelenz Jan 20, 2021
@brad4d
Copy link
Contributor

brad4d commented Mar 31, 2021

I've just added a wiki page to explain generating chunks for dynamic loading, including a section on how to load the chunks.

https://github.com/google/closure-compiler/wiki/Chunk-output-for-dynamic-loading

I got permission form @ChadKillingsworth to use his stackoverflow answer as a starting point.

@brad4d brad4d closed this as completed Mar 31, 2021
@DavidANeil
Copy link
Contributor Author

Okay, I've figured out what was going wrong. The goog.module.ModuleLoader implementation eventually calls eval with the source string. If it instead appends a <script> element to the document with the source, then everything works as expected.

I guess we'll just need to set our own implementation for the loader to get this working.

I honestly had no clue that <script> and an indirect eval worked differently in this way.

Example for thoroughness' sake...
The following works just fine:

<html>
  <head>
    <script>
        function loadScript(source) {
            const scriptElt = document.createElement('script');
            scriptElt.appendChild(document.createTextNode(source));
            document.head.appendChild(scriptElt);
        }
        const mainSource = 'const x = 5;';
        loadScript(mainSource);

        const otherSource = 'console.log(x+5);';
        loadScript(otherSource);
    </script>
  </head>
</html>

While this produces a runtime error:

<html>
  <head>
    <script>
        const indirectEval = eval;

        const mainSource = 'const x = 5;';
        indirectEval(mainSource);

        const otherSource = 'console.log(x+5);';
        indirectEval(otherSource);
    </script>
  </head>
</html>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brad4d brad4d

Labels
triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@brad4d @concavelenz @ChadKillingsworth @rishipal @DavidANeil