google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.41k stars 1.15k forks source link

[FEATURE] Support dynamic import #2770

Closed dtruffaut closed 3 years ago

dtruffaut commented 6 years ago

Support dynamic import :

https://developers.google.com/web/updates/2017/11/dynamic-import

module.js :

export const foo = _ => {
  console.log('foo');
}

main.js :

const main = async _ => {

  const m = await import('module.js');
  m.foo();

  (await import('module.js')).foo();

}

main();

Should display :

foo
foo

The thing is about supporting 'export' and 'import' keywords.

blickly commented 6 years ago

Thanks for filing the tracking bug.

Just want to confirm that the feature we're tracking here should be the currently stage 3 dynamic import syntax, not top-level await syntax.

dtruffaut commented 6 years ago

Yes !

ChadKillingsworth commented 6 years ago

@blickly There is already both Chrome and Safari support for this feature. The hard part is figuring out exactly what the compiler should do here. But parsing would be a good first step...

dtruffaut commented 6 years ago

Any news on this ? At least, just allow the parsing ?

maxmilton commented 6 years ago

Would be amazing to see parser support for import().

Spent the morning trying to add Closure Compiler as the minifier in https://github.com/MaxMilton/sapper-template-rollup/tree/feat/use-closure-compiler but in the end it turns out the import() can't be parsed and the compiler bails completely. In this project the import function is polyfilled so we don't need the compiler to do anything special. This is now a blocker for adding Closure Compiler support :sweat:

lauraharker commented 5 years ago

Created Google internal issue http://b/122961838 for this request.

Tonsil commented 5 years ago

Has there been any movement on this recently? Asking because we're getting into a project for which it would be extremely useful. I figured I'd ask before we decide to fork and fix it ourselves.

dtruffaut commented 5 years ago

Most of JS developers now use Rollup instead of Closure Compiler.

Rollup does a great job in handling await import instructions and moving all your imports at the same level on the disk.

As a result, code splitting leads to faster load times.

File size matters, but code splitting really is on another level. The benefits are impressive.

In addition, Rollup understands ESNext and respects property names, which leads to fewer bugs.

As of July 2019, I would recommend Rollup over Closure Compiler.

maxmilton commented 5 years ago

Although rollup and closure compiler do have some overlap in that they can both handle JS bundling, their functionality is not mutually exclusive. If you use rollup you can also use closure compiler with a plugin like @ampproject/rollup-plugin-closure-compiler.

Rollup is an excelent module bundler and closure compiler is an excelent JS minifier. If you want the most performant code, why not use both?

rjakobsson commented 5 years ago

@Tonsil, did you fork?

Tonsil commented 5 years ago

@Tonsil, did you fork?

No, we punted on it. We wanted to dynamically import a library because it's on the large side and not needed in all cases. For now, we decided instead to keep that code as a separate file (for other reasons, like not wanting to have to rewrite it just yet). Our workaround was to have our modular code write out a script tag on the page and pull it in that way. If/when we do decide to fork, I'll let you know.

brad4d commented 5 years ago

"Support" can mean at least 2 different things for dynamic import here.

  1. Pass a dynamic import call from the source all the way through so it's still there in the compiled version, in order to load some ES module that isn't part of the JS we're compiling.
  2. Recognize that a dynamic import statement is referring to another file that is being compiled and do some kind of magic code-splitting based on this to break the output into multiple chunks that dynamically load each other as needed.

People building big applications definitely want the code splitting, but there's a lot to work out there.

I'd really like to know how much people care about importing foreign ES modules (number 1). closure-compiler doesn't support doing that even with static import now. Do people care, or does that not matter because everyone wants to compile all parts of their application together at once anyway?

I would like to advocate for implementing import of foreign ES modules, but that will be a hard sell if I cannot show that a significant number of people really want it and would benefit from it.

sjrd commented 5 years ago

I'd really like to know how much people care about importing foreign ES modules (number 1). closure-compiler doesn't support doing that even with static import now. Do people care, or does that not matter because everyone wants to compile all parts of their application together at once anyway?

FWIW, the Scala.js compiler cares, and therefore, indirectly, a lot of Scala.js users. That's because Scala.js uses Closure to optimize the Scala.js code but not its potential JS dependencies.

brad4d commented 5 years ago

I also have this related question:

Do people want closure-compiler to be able to generate output that functions as an ES module and can be imported JS code the compiler never sees?

sjrd commented 5 years ago

Yes. Scala.js would also like that. I'd love to be able to remove this line of code: https://github.com/scala-js/scala-js/blob/da1627b4715c4059c742c511ac02a285174b36c6/linker/jvm/src/main/scala/org/scalajs/linker/backend/closure/ClosureLinkerBackend.scala#L50

adrianholovaty commented 4 years ago

@brad4d wrote:

"Support" can mean at least 2 different things for dynamic import here.

  1. Pass a dynamic import call from the source all the way through so it's still there in the compiled version, in order to load some ES module that isn't part of the JS we're compiling.

  2. Recognize that a dynamic import statement is referring to another file that is being compiled and do some kind of magic code-splitting based on this to break the output into multiple chunks that dynamically load each other as needed.

People building big applications definitely want the code splitting, but there's a lot to work out there.

I'd really like to know how much people care about importing foreign ES modules (number 1). closure-compiler doesn't support doing that even with static import now. Do people care, or does that not matter because everyone wants to compile all parts of their application together at once anyway?

My two cents (as a longtime Closure Compiler user, and somebody who would like it to support dynamic module import): Number 2 is much more important. I wouldn't personally need to dynamically load JS that's not being compiled by my own workflow.

tmb-github commented 4 years ago

@MaxMilton A brute-force workaround for including dynamic import() in JavaScript to be compiled with the Closure Compiler is temporarily to rename every import() statement to dynamicImport() and then compile, then convert every dynamicImport() back to import() in the resulting compilation. This is admittedly incredibly dreadful, but IT WORKS (at least in my experiments with "SIMPLE OPTIMIZATIONS"). The compiler leaves dynamicImport() as-is (i.e., unrenamed), and so you can search-for and replace it. (Yes, this is not an ideal solution, but it works in my tests. See what you get.)

stevendwood commented 4 years ago

erm, you could also just wrap the dynamic import if you have control over all the code you are building like Function("return import('" + moduleName + "')")() not very attractive either but works.

ChadKillingsworth commented 4 years ago

Design document for support: https://docs.google.com/document/d/1thSbGV5i96jltVwn76KYrPkWoNc-yj_KU3HtBGUCgMc/edit

ChadKillingsworth commented 3 years ago

This is fully supported as of the 20210505 version

tmb-github commented 3 years ago

The online interface to the Closure Compiler (https://closure-compiler.appspot.com) is still throwing errors when dynamic import statements are encountered:

JSC_DYNAMIC_IMPORT_USAGE: Dynamic import expressions cannot be transpiled. at line 2225 character 28
                            import(pageObj.mjs).then(function ({default: obje...

When will it be updated to the 20210505 version?

ChadKillingsworth commented 3 years ago

Allowing dynamic imports is enabled by default in the command line version, but the option is off by default for the compiler options. This would have to be specifically enabled for the appspot instance.

tmb-github commented 3 years ago

@ChadKillingsworth When I run the 20210505 version locally at the command line, the closure compiler still fails to work with dynamic imports.

Using this command:

java -jar closure-compiler-v20210505.jar --js=input.js --js_output_file=output.js --compilation_level=SIMPLE --rewrite_polyfills=false

...I get the following error:

input.js:2225:28: WARNING - [JSC_DYNAMIC_IMPORT_ALIASING_REQUIRED] Dynamic Import Expressions should be aliased for for language level
  2225| import(pageObj.mjs).then(function ({default: object}) {
        ^^^^^^^^^^^^^^^^^^^

ERROR - [JSC_FEATURES_NOT_SUPPORTED_BY_PASS] Attempted to run pass "rewriteExponentialOperator" on input with features it does not support. Running pass anyway.
Unsupported features: [Dynamic module import]

1 error(s), 1 warning(s)

The same error plus an additional "Failed to load module" error occurs if I provide full path info in the import argument, i.e., import('.\pageObj.mjs').then(function ({default: object}) {

Per the error message, I'm unclear on how to "alias" a dynamic import expression for language level.

Also, the documentation you linked previously contains this text:

Proposal: Resolve the module referenced by the dynamic import statement when possible and emit a warning which can be suppressed when not possible.

Examples:

import('./module1.js'); // fully resolvable at build time import(MY_MODULE); // not resolvable - issue warning import(FOO).then((data) => { // not resolvable - issue warning data['external'](); });

import('./not/in/compilation.js'); // not resolvable - issue warning

This suggests that the import in my code is not resolvable. It doesn't look like the closure compiler is really supporting dynamic imports as they are used in actual code (with promise-style .then(), etc.). If it is supporting them with code tweaking, etc., could you clarify?

Many thanks!

EDIT: In reviewing my code, pageObj.mjs is, in fact, an object containing a string that holds the relative path to the module to be imported dynamically. So, that's not a problem. But the compilation problem persists.

ChadKillingsworth commented 3 years ago

Take a look at https://github.com/google/closure-compiler/wiki/JS-Modules#dynamic-import-expressions and see if that helps. If not, let me know and I'll clarify the documentation.

tmb-github commented 3 years ago

@ChadKillingsworth No such luck for me. The text that states this gives hope:

Dynamic import specifiers can also be arbitrary expressions (ex import(modulePathVar)). In this case, the compiler can’t resolve the module. The returned promise will resolve to an unknown type and no warning will be issued. The compiler assumes you know what you are doing here and the imported module is external.

But even if I save the name of the imported module in a variable and code import(myVar).then( it still throws an error.

If I understand the --dynamic_import_alias switch correctly, you must rename all of your import() functions to an alias, say import_(), before compiling. That does work, but the resulting compilation is compromised.

My modules typically have this format:

var aaa;
var bbb;
var ccc;

aaa = function () {};
bbb = function () {};
ccc = function () {};

export default Object.freeze({
    aaa,
    bbb,
    ccc
});

After renaming import() to import_() throughout the code, the resulting compilation is:

var aaa$$module$input,bbb$$module$input,ccc$$module$input;

aaa$$module$input = function () {};
bbb$$module$input = function () {};
ccc$$module$input = function () {};

var $jscompDefaultExport$$module$input=Object.freeze({aaa:aaa$$module$input,bbb:bbb$$module$input,ccc:ccc$$module$input}),module$input={};module$input.default=$jscompDefaultExport$$module$input;

I want it to preserve my variable names and object.freeze at the end. I've worked around this previously by renaming my import() statements to dynamicImport(), and then recording the variable names and terminating object.freeze, restoring them after the Closure Compiler is finished. That works, but, of course, it's a series of steps I'd like to avoid.

Many thanks in advance for your help.

ChadKillingsworth commented 3 years ago

If I understand the --dynamic_importalias switch correctly, you must rename all of your import() functions to an alias, say import(), before compiling.

That's backwards. The compiler will rename dynamic imports for you with this flag. Your source code shouldn't need touched. So with --dynamic_import_alias=dynamicImport:

source:

import('./my-module.js')

becomes:

dynamicImport('./my-module.js')

As for preserving your exported names, that's a use case that's not yet supported

tmb-github commented 3 years ago

@ChadKillingsworth Thanks! Okay, understanding that, here's the error I now get.

Provided this for input.mjs:

var aaa;

aaa = function () {
    var myModule = './myModule.mjs';
    import(myModule).then(function ({default: object}) {
        console.log(object);
    });
};

export default Object.freeze({
    aaa
});

And provided this execution:

java -jar closure-compiler-v20210505.jar --js=input.mjs --js_output_file=output.mjs --compilation_level=SIMPLE --rewrite_polyfills=false --dynamic_import_alias=dynamicImport

Then the error produced is:

java.lang.NullPointerException: NAME dynamicImport 5:4  [length: 16] [source_file: input.mjs]
        at com.google.javascript.jscomp.jarjar.com.google.common.base.Preconditions.checkNotNull(Preconditions.java:897)
        at com.google.javascript.jscomp.RemoveUnusedCode.getVarForNameNode(RemoveUnusedCode.java:891)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNameNode(RemoveUnusedCode.java:746)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:571)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1301)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:803)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:498)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNormalOrOptChainGetProp(RemoveUnusedCode.java:629)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:577)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1301)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseCall(RemoveUnusedCode.java:803)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:498)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1301)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseNode(RemoveUnusedCode.java:581)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseChildren(RemoveUnusedCode.java:1301)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseFunction(RemoveUnusedCode.java:1442)
        at com.google.javascript.jscomp.RemoveUnusedCode.access$1100(RemoveUnusedCode.java:83)
        at com.google.javascript.jscomp.RemoveUnusedCode$Continuation.apply(RemoveUnusedCode.java:1804)
        at com.google.javascript.jscomp.RemoveUnusedCode.traverseAndRemoveUnusedReferences(RemoveUnusedCode.java:412)
        at com.google.javascript.jscomp.RemoveUnusedCode.process(RemoveUnusedCode.java:386)
        at com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process(PhaseOptimizer.java:317)
        at com.google.javascript.jscomp.PhaseOptimizer$Loop.process(PhaseOptimizer.java:462)
        at com.google.javascript.jscomp.PhaseOptimizer.process(PhaseOptimizer.java:232)
        at com.google.javascript.jscomp.Compiler.performOptimizations(Compiler.java:2442)
        at com.google.javascript.jscomp.Compiler.lambda$stage2Passes$1(Compiler.java:791)
        at com.google.javascript.jscomp.CompilerExecutor.lambda$runInCompilerThread$0(CompilerExecutor.java:101)
        at java.util.concurrent.FutureTask.run(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
        at java.lang.Thread.run(Unknown Source)

Is there a missing step or CLI flag?

ChadKillingsworth commented 3 years ago

Provide a definition for dynamicImport as an extern. That's what is missing. We need a better error for this case.

tmb-github commented 3 years ago

@ChadKillingsworth What would go in the extern file for this simple case, and how would that file be passed to the executable on the command line? I see the documentation at https://developers.google.com/closure/compiler/docs/externs-and-exports, but I'm unclear how to apply it to the code provided above.

ChadKillingsworth commented 3 years ago

This would be a good question for the mailing list or for stackoverflow.com