The dynamic import expression import('./path') gives an error

masaladi commented 1 year ago

I have two questions:

First, in 2.21.0, I added a parameter that set it to true for the test case, but when I upgraded to 2.22.0 and above, it gave me an error 微信截图_20230403112132

Second, in the 2.21.0 version, according to the following set of test cases, if the < closureAllowDynamicImport > when set to false, the corresponding < closureDependencyEntryPoints > file path corresponding is true, When I try to use dynamic-imports-false.js, the compression throws an error https://github.com/blutorange/closure-compiler-maven-plugin/blob/master/src/test/resources/projects/allowdynamicimport/pom.xml

blutorange commented 1 year ago

Thanks very much for reporting this issue, let me try and see if I can reproduce it with the tests. For reference, could you post the errors it is giving you?

masaladi commented 1 year ago

Yes, my code is the code of the reference use case, and the error message is shown in the second screenshot below

blutorange commented 1 year ago

Alright, now I had some time to think about this. I'm still not quite sure I understand your problem, but let me try to answer your questions.

First of all, yes, this line from the test (https://github.com/blutorange/closure-compiler-maven-plugin/blob/master/src/test/resources/projects/allowdynamicimport/pom.xml#L38) is wrong, it should say

file:dynamic-imports-false.js

I changed that now, but the test still passes and does not result in a build error.

Secondly, the release 2.22.0 release of this Maven plugin updated closure compiler from v20210202 to v20211006. I found this issue on the closure compiler repo which might be related to your case:

https://github.com/google/closure-compiler/issues/3941#issuecomment-1128917884

One thing you can do to check if this is a closure compiler issue: Download the closure compiler JAR and run it directly against your code, with the same options from your pom.xml. If that results in the same error, it's a closure compiler issue I can't do much about unfortunately.

Thirdly, regarding the error message you posted. I can't see it in that screenshot, but when you scroll to the right, does the file list after Failed to process the source files include util.js ? The error sounds like it simply can't find the imported file.

Either way, I can't reproduce the error you mentioned. It might or might now be a closure compiler issue. If you are allowed to, it would be great if you could share your test project that's giving you the error, then I can take a look if there's anything I can change in the plugin to fix it.

masaladi commented 1 year ago

Thank you very much for your answer. I have done more tests and checked my configuration according to your reminder, but I still did not find the problem. Now I have built a test project(https://github.com/masaladi/closure-compiler-test) according to the configuration of my project, but when I use maven pakcage, there is still an error. Please help to find the time to check what causes it. Thanks again.

the full error message: https://github.com/masaladi/closure-compiler-test/blob/main/error-message.md?plain=1

blutorange commented 1 year ago

Thanks for uploading the project. So first of all, it seems that closureAllowDynamicImport isn't the issue. The problem appears to be skipMerge, when I set it to false, it builds successfully. I'm not sure if closure compiler supports minifying files while leaving imports untouched, let me check. Just to make sure: you really want to create separate files that still contain import {...} from "..." statements and not combine the files into a single file?

masaladi commented 1 year ago

Yes, for now I just want to create separate files without merging them.

blutorange commented 1 year ago

Shorter answer:

Unless I'm missing something, closure compiler does not support minifying individual files while preserving static ESM imports
- Related: https://github.com/google/closure-compiler/issues/3740#issuecomment-751790874
- Since this plugin is just a wrapper around closure compiler, there's nothing I can do about it currently.
Preserving dynamic imports is broken in current closure compiler versions.
- https://github.com/google/closure-compiler/issues/3941#issuecomment-1128917884
- The test you mentioned only works by accident since it sets merge to false, so closure compiler can resolve the dynamic import (even though it does not need to since it preserves it in the output)

Longer answer:

Dynamic imports

As mentioned in the closure compiler issue I linked above, that seems to be a bug in recent versions of closure compiler. You can test this by running closure compiler on your test project directly.

With v20210202 it still works:

java -jar ~/Downloads/closure-compiler-v20210202.jar --js src/main/resources/public/js/lib/test/main.js --allow_dynamic_import --js_output_file target/generated-resources/public/main.js --language_in ECMASCRIPT_NEXT --language_out ECMASCRIPT_NEXT

With v20230228, it does not work anymore:

java -jar ~/Downloads/closure-compiler-v20230228.jar --js src/main/resources/public/js/lib/test/main.js --allow_dynamic_import --js_output_file target/generated-resources/public/main.js --language_in ECMASCRIPT_NEXT --language_out ECMASCRIPT_NEXT

And as to why the test is working, this is also related to the skipMerge. The test sets skipMerge to false, so closure compiler knows about all input files and can resolve the dynamically imported module internally. When the import is changed to some unknown path, the test fails as well.

Static ESM imports

After checking the flags and options again I don't think closure compiler supports minifying individual files that statically import other files, at least not without transforming the static import statements. See also for example this issue

https://github.com/google/closure-compiler/issues/3740#issuecomment-751790874

The --allow_dynamic_import flag is meant only for, well, dynamic imports, not for static imports.

The way closure compiler works is that it takes all input files and compiles them into a single file. When you set merge to false, this plugin iterates over each file individually, and calls closure compiler once for each file, like this:

java -jar ~/Downloads/closure-compiler-v20230228.jar --js src/main/resources/public/js/lib/test/main.js --js_output_file target/generated-resources/public/main.js

So now, when main.js gets compiled, closure compiler does not know about utils.js and complains about it.

If you do add utils.js as an input file, both files will get merged.

java -jar ~/Downloads/closure-compiler-v20230228.jar --js src/main/resources/public/js/lib/test/main.js --js src/main/resources/public/js/lib/test/utils.js --js_output_file target/generated-resources/public/merged.js

blutorange commented 1 year ago

In other words, this appear to be a limitation of closure compiler, or rather, closure compiler's main intended goal is to read the entirety of the source code so that it can perform optimizations and then output a single file.

Closure compiler does have a Code Splitting feature which can split the output into multiple files. You can try it out directly with the closure compiler JAR. If this is what you need, I could add support for code splitting to this plugin.

Another alternative to consider is the frontend-maven-plugin, which I am using in many project. It lets you run npm or yarn, so you can use any bundler from the JavaScript eco system (e.g. webpack, esbuild, parcel etc.)

blutorange commented 1 year ago

This might now be possible with <closureChunkOutputType>ES_MODULES</closureChunkOutputType> https://github.com/google/closure-compiler/wiki/Chunk-output-for-dynamic-loading

blutorange / closure-compiler-maven-plugin