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.

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

Enable Closure when emitting an ES module. #3893

Open
Kesanov opened this issue Dec 3, 2019 · 34 comments
Open

Enable Closure when emitting an ES module. #3893

Kesanov opened this issue Dec 3, 2019 · 34 comments
Labels
optimization Optimization only. Does not affect semantics or correctness.

Comments

@Kesanov
Copy link

Kesanov commented Dec 3, 2019

When the ES module option is active:

scalaJSLinkerConfig ~= { _.withModuleKind(ModuleKind.ESModule) }

the js file obtained trough fullOptJS isn't minified and project-opt.js is only 3% smaller in size than project-fast-opt.js

@Kesanov Kesanov changed the title ES module disables minification. ES module disables minification for fullOptJS. Dec 3, 2019
@sjrd sjrd added the optimization Optimization only. Does not affect semantics or correctness. label Dec 3, 2019
@sjrd
Copy link
Member

sjrd commented Dec 3, 2019

Yes, this is a known limitation of the version of Closure that we use. A PR that upgrades GCC and manages to make this work would be welcome.

@ngbinh
Copy link

ngbinh commented Dec 5, 2019

@sjrd does scala.js 1.X have the same issue?

@sjrd
Copy link
Member

sjrd commented Dec 5, 2019

Yes, it does.

@ngbinh
Copy link

ngbinh commented Dec 5, 2019

I thought this commit 5c8f049 has already upgraded GCC to a very recent version and that version has ES6 support already?

@sjrd
Copy link
Member

sjrd commented Dec 5, 2019

There is support for ES 6 (official name ES 2015) except modules. We do enable GCC for ES 2015 code, which is even the default in Scala.js 1.x.

We disable GCC when emitting an ES module.

@ngbinh
Copy link

ngbinh commented Dec 5, 2019

Thanks for the explanation.

@sjrd sjrd changed the title ES module disables minification for fullOptJS. Enable Closure when emitting an ES module. Jan 22, 2020
@gzm0 gzm0 assigned gzm0 and unassigned gzm0 Feb 26, 2020
@gzm0 gzm0 linked a pull request Feb 27, 2020 that will close this issue
@gzm0 gzm0 removed a link to a pull request Feb 27, 2020
@gzm0 gzm0 self-assigned this Feb 29, 2020
@gzm0
Copy link
Contributor

gzm0 commented Apr 28, 2020

An update on this: I have a branch with the closure AST transformers built to support module related constructors (import, export). However, I cannot get closure to simply copy the modules we import from: It tries to validate them, and as such needs much more environmental information than we are able to provide.

I have not yet found a way to disable this behavior.

@gzm0
Copy link
Contributor

gzm0 commented Apr 28, 2020

@gzm0
Copy link
Contributor

gzm0 commented Apr 28, 2020

In practical terms, the problem is that we cannot configure this snippet in Compiler.java:

https://github.com/google/closure-compiler/blob/9b72d53363a9dd5d8582e27044524027cb3655ce/src/com/google/javascript/jscomp/Compiler.java#L1640-L1676

What we would want is a ModuleLoader / ModuleResolver that simply resolves to its input (full story is a bit more complicated, but that's the higher level idea).

@sjrd
Copy link
Member

sjrd commented Apr 28, 2020

I think the discussion at google/closure-compiler#2770 means that it's simply not supported by Closure.

@gzm0
Copy link
Contributor

gzm0 commented Apr 28, 2020

Ah, yes, indeed.

@gzm0
Copy link
Contributor

gzm0 commented May 18, 2020

A quick update on this: I have tried to reach out to the closure folks Google-internally to see if they would be willing to accept a contribution that enables this.

The higher level idea was to add a PassthroughModuleResolver (extends ModuleResolver) which simply returns the moduleAddress as-is. Add a new value PASSTHROUGH to ResolutionMode which uses the new resolver.

Unfortunately I didn't get a reply back so far. @sjrd do you have contacts we could reach out to?

It seems at this point we're pretty much blocked on this, unless we are willing to expose all the relevant options in the linker config and force users to set them according to their environment.

@sjrd
Copy link
Member

sjrd commented May 18, 2020

I'm afraid I don't any direct contact in the Closure team. However, I've a good experience reporting issues and sending PRs to fix things. In this case it's a feature rather than a bug fix, so the story might be different, but I think there's a good chance they would consider it.

@gzm0
Copy link
Contributor

gzm0 commented Sep 10, 2020

Unfortunately, the closure team has confirmed (Google-internally) that they cannot accept a contribution of this form. Mainly because it is just too much of a stretch from how GCC works.

For Scala.js, this means we either need to try and work around this or close this as wontfix.

@gzm0
Copy link
Contributor

gzm0 commented Sep 10, 2020

The only thing I can think about is replace imports with a function call to a "magic" function, then run GCC, then replace the function calls again with imports. In the process, we of course somehow need to make sure GCC doesn't optimize these "too much". Not sure how feasible this is :-/

@sjrd
Copy link
Member

sjrd commented Sep 10, 2020

That workaround could work for dynamic imports, but what about static imports of ES modules? I guess we would have to not include them in the code being compiled by GCC, but instead exposed as of they were global variables, and then patch up the imports ourselves at the beginning of the file?

@gzm0
Copy link
Contributor

gzm0 commented Sep 10, 2020

I did mean this for static imports, but then probably, indeed, the likelihood of GCC optimizing it is too high. I meant something like:

import { a as b } from "foo.js";
// translate to
const b = _my_magic_function("a", "foo.js");

My hope was that we can get GCC to rename the local identifiers of what is imported.

@sjrd
Copy link
Member

sjrd commented Sep 10, 2020

Hum... It could work. As an external reference, GCC must keep the function call for sure, and give the right values as parameters. But it could move the call around, or separate the literals from the call, etc.

@gzm0
Copy link
Contributor

gzm0 commented Jun 6, 2021

From gitter: google/closure-compiler#2770 (comment) looks like GCC might actually support this now.

@gzm0
Copy link
Contributor

gzm0 commented Jun 6, 2021

OK. So a bit of clarification: What Scala.js needs is support for what GCC calls "Foreign Modules" (modules that are not known at the time of compilation). Scala.js needs this for both dynamic and static imports.

The issue in question is about dynamic imports (import("foo") as opposed to import * as x from "foo";). It seems that the support for dynamic imports has some support for foreign modules. It is unclear from the discussion, if this support has been retrofitted to static imports (or whether it has been built in the meantime).

@armanbilge
Copy link
Member

Nearly a year later, a polite bump on this :) I know the requests just don't stop, sorry!

Thanks to SmallModulesFor ESModules are now all the rage for development and we have nice examples like this:
https://github.com/sjrd/scalajs-sbt-vite-laminar-chartjs-example

I believe lack of GCC support for ESModules is (hopefully) the last sore spot, specifically relating to production deploys. In my experience GCC makes significant optimizations to the generated JS that IIUC are impossible to replicate after-the-fact (e.g. see http4s/http4s-dom#119 (comment)).

Currently the only way to use GCC in production is to build an alternative bundling pipeline that is based on CommonJS, alongside the ESModule-based development bundling pipeline.

Thanks :)

@sjrd
Copy link
Member

sjrd commented May 20, 2022

There's apparently this thing now in GCC:
https://github.com/google/closure-compiler/blob/be4a58341ee03da20fa2104ba90792745ccb3cfb/src/com/google/javascript/jscomp/CompilerOptions.java#L1164-L1171
We could try and set it to false in ClosureLinkerBackend.closureOptions and see what happens. 🤷‍♂️

@gzm0
Copy link
Contributor

gzm0 commented May 22, 2022

Setting that option still seems to make it fail:

[error] JSC_INVALID_MODULE_PATH. Invalid module path "foo.js" for resolution mode "BROWSER" at virtualfile:scala.js-ir line (unknown line) : (unknown column)

For posterity, here's how you translate ImportNamespace:

      case ImportNamespace(binding, from) =>
        new Node(Token.IMPORT,
          new Node(Token.EMPTY),
          setNodePosition(Node.newString(Token.IMPORT_STAR, binding.name), binding.pos.orElse(pos)),
          transformExpr(from),
        )

@yurique
Copy link

yurique commented Aug 22, 2022

@gzm0 I've tried playing with it a bit.

To be honest I'm not sure if I know what I'm doing =); nevertheless, I managed to get the JSC_INVALID_MODULE_PATH. Invalid module path "foo.js" error.

This error happens when we try to do import from "file.js" with the BROWSER resolution mode enabled (which is by default, apparently) (described here).

Changing the import to be import from "./file.js" (note the ./) makes the error go away.

There is also the NODE resolution mode (it is also described at the page linked above):

options.setModuleResolutionMode(ResolutionMode.NODE)

It will also want the ./ for the "local" imports, but it will go into the node_modules if there is no ./ (/cc @armanbilge).

I also tried another option: Transpiling Dynamic Import

options.setDynamicImportAlias("__scala_js_import__")

Which is supposed to make closure "support" dynamic imports even when the output is lower than ECMASCRIPT_2020:

The presence of this flag instructs the compiler to allow dynamic imports even when the output language is lower than ECMASCRIPT_2020

But it didn't work for me - it complains:

SEVERE: test.js:5:6: ERROR - [JSC_LANGUAGE_FEATURE] This language feature is only supported for ECMASCRIPT_2020 mode or better: Dynamic module import.
  5|       import("./bar.js").then((bar) => bar.q(z))

I think this is why: google/closure-compiler#3941

Not sure if this is helpful at all. I hope it is :)


P.S. When running this:

  import com.google.javascript.jscomp.{
    SourceFile => ClosureSource,
    Compiler => ClosureCompiler,
    CompilerOptions => ClosureOptions,
    _
  }

  val options = new ClosureOptions
  options.setPrettyPrint(true)
  CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options)

  options.setLanguage(ClosureOptions.LanguageMode.ECMASCRIPT_2015)
  options.setWarningLevel(DiagnosticGroups.GLOBAL_THIS, CheckLevel.OFF)
  options.setWarningLevel(DiagnosticGroups.DUPLICATE_VARS, CheckLevel.OFF)
  options.setWarningLevel(DiagnosticGroups.CHECK_REGEXP, CheckLevel.OFF)
  options.setWarningLevel(DiagnosticGroups.CHECK_TYPES, CheckLevel.OFF)
  options.setWarningLevel(DiagnosticGroups.CHECK_USELESS_CODE, CheckLevel.OFF)

  val ScalaJSExterns = 
    """
    function callMe(x) {};    
    """

    val chunk = new JSChunk("chunk1")
    chunk.add(
      ClosureSource.builder().withPath("test.js").withContent("""
      import { x as y } from './foo.js'
      const z = y();
      callMe(z);
      """
    ).build())
    chunk.add(
      ClosureSource.builder().withPath("foo.js").withContent("""
      export const x = () => "Hi!"
      """
    ).build())

  compiler.compileModules(
    Arrays.asList(ClosureSource.fromCode("ScalaJSExterns.js", ScalaJSExterns)),    
    Arrays.asList(chunk),
    options
  )

  val source = compiler.toSource
  println(source)

it prints:

'use strict';
callMe("Hi!");

P.P.S.

I also tried publishLocaling the compiler and linking a simple project with these settings:

      _.withModuleKind(ModuleKind.ESModule)
      .withESFeatures(_.withESVersion(ESVersion.ES5_1))
      .withModuleSplitStyle(ModuleSplitStyle.SmallModulesFor(List("myapp")))
      .withClosureCompiler(true) 

Had to add this to the ClosureAstTransformer (as well as the case ImportNamespace(binding, from) => ... that @sjrd posted above):

      case Export(bindings) => 
        val specs = new Node(Token.EXPORT_SPECS)
        bindings.foreach { case (ident, exportName) =>
          specs.addChildToBack(
            new Node(
              Token.EXPORT_SPEC, 
              Node.newString(ident.name),
              Node.newString(exportName.name)
            )
          )
        } 
        new Node(Token.EXPORT, specs)

I think it worked (although I messed something and it looks like fastLinkJS and fullLinkJS switched roles :) ). But in this small project there are no imports, so that doesn't say anything.

Also when I tried .withModuleSplitStyle(ModuleSplitStyle.SmallestModules) (hoping to get those imports) scalajs refused to link it:

(Compile / fastLinkJS) java.lang.IllegalArgumentException: requirement failed: Cannot use multiple modules with the Closure Compiler

@OndrejSpanel
Copy link

What is the current status? Does it mean fullOpt / fullLink is limited with ModuleKind.ESModule?

Could the limitation be documented, perhaps at https://www.scala-js.org/doc/project/module.html? As I read it now, there is no mention of any limitations at all.

@sjrd
Copy link
Member

sjrd commented Jun 23, 2023

The status is still the same as when this issue was opened. Closure is disabled with ESModule.

@OndrejSpanel
Copy link

Can some documentation / warning be placed the the module.html page then?

Background: As a result of dropped plain script support (mrdoob/three.js#25435) I am now changing the way three.js is included in my application . I was considering whether to include it now as a common module or ES module, and given ES modules are easier to use in the browser and given even Common JS module script was intended to be removed originally (mrdoob/three.js#25341) I was favoring ES modules, but having fullOpt severely limited is a no-go for ES module version. I found this topic discussing the limitation only by chance.

@gzm0
Copy link
Contributor

gzm0 commented Jun 28, 2023

Can some documentation / warning be placed the the module.html page then?

That seems like a fair ask. Feel free to open a PR or file an issue against scala-js-website.

@arashi01
Copy link

arashi01 commented Sep 8, 2023

A quick update on this: I have tried to reach out to the closure folks Google-internally to see if they would be willing to accept a contribution that enables this.

The higher level idea was to add a PassthroughModuleResolver (extends ModuleResolver) which simply returns the moduleAddress as-is. Add a new value PASSTHROUGH to ResolutionMode which uses the new resolver.

Unfortunately I didn't get a reply back so far. @sjrd do you have contacts we could reach out to?

It seems at this point we're pretty much blocked on this, unless we are willing to expose all the relevant options in the linker config and force users to set them according to their environment.

@gzm0 @sjrd How feasible would it be for us to maintain our own downstream fork. Similar to how the sbt team ended up doing with ivy if I'm not mistaken.

I'm not familiar with the internals off GCC but if the areas we're touching are pretty stable already upstream then it should mainly involve merging in future upstream changes. I'm sure a few people (including myself) are happy to dedicate time towards that if need be.

@sjrd
Copy link
Member

sjrd commented Sep 8, 2023

We haven't studied the feasibility of having our own fork. The fork itself is one thing, but most importantly we would need to implement the support in the first place. It's unclear that doing that would be any less effort than implementing global property name minimization in our toolchain instead.

@gzm0
Copy link
Contributor

gzm0 commented Sep 16, 2023

IMO the idea of forking or using more low level APIs of GCC is an interesting one worth investigating. But I agree with sjrd that we have no chance to guess the maintenance effort before doing the work of figuring out how such an integration could look in practice.

@v6ak
Copy link

v6ak commented Oct 20, 2023

I've had two ideas:

  1. Quite hacky way: rewrite imports to requires, let GoCC proceed and rewrite it back. This works in my PoC: Reduce JS bundle size vite-plugin-scalajs#14 (comment)

  2. (EDIT: probably a dead-end, as @nocompile doesn't seem to be used even in GoCC tests.) Less hacky way: Generate module stubs with @nocompile. I've tried to do it with some minimal modules (not generated by Scala.js) and I am currently getting a NullPointerException. But maybe someone has an idea how to do it properly. There are two JS files + shell script for compilation + output: https://gist.github.com/v6ak/c00dc292ae4faf93c8013ceb3fb8c5da

@v6ak
Copy link

v6ak commented Oct 23, 2023

I've tried to use chunks. It also seems to be a dead-end for general case (but might be the way to go for js.dynamicImport support):

  • It emits imports, even static imports, but chunk names cannot contain some characters like slashes. This implies a postprocessing would be needed in order to emit import * as foo from 'bar/baz';. But if we accept postprocessing, we can go an easier way… (Probably not an issue for js.dynamicImport…)

  • I was able to specify just one dependency, not two. Maybe there is a way, but I don't see it. (Probably not an issue for js.dynamicImport…)

  • It doesn't respect module boundaries, which is a no-go for 3rd party modules that are expected to be untouched.* (Probably not an issue for js.dynamicImport…) This is also mentioned in the chunks documentation

    Keep in mind that the compiler can and does move code from one chunk into other chunk output files if it determines that it is only used by that chunk.

  • It sometimes behaves a little weird, e.g. my example with dynamic import emits both dynamic and static import. It might be either a bug or a feature (e.g., GoCC realizes that the dynamic import will be needed immediatelly). I haven't digged deeper in the weirdnesses, as this seems to be useless for basic ES modules support anyway. One might be interested later (when implementing js.dynamicImport), though.


As a result, I still don't see any other practical way that 1. rewriting imports to function calls before GoCC is called and 2. rewriting function calls to imports afterwards. It works well** in my hacky (regex-based) PoC and I believe it would work well in the general case if regex hacks are replaced by AST transformations. The reason why I am not going this way is that it probably wouldn't be accepted.


*) One might be tempted to use @noinline, but it is experimental and incomplete according to the documentation.
**) It breaks source maps, but when we replace regex-based transformations by AST-based transformations, it should keep them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Optimization only. Does not affect semantics or correctness.
Projects
None yet
Development

No branches or pull requests

9 participants