Scala.js 1.0.0 unnecessary generates imports for parent classes

[oyvindberg]

As mentioned in ScalablyTyped/Converter#131 Scala.js 1.0.0 ends up including imports of superclasses unnecessarily when referencing a class. I'll copy in the example:

object Main {
  @JSImport("rsocket-websocket-client/RSocketWebSocketClient", JSImport.Default)
  @js.native
  class Parent extends js.Object

  @JSImport("rsocket-websocket-client", JSImport.Default)
  @js.native
  class Child extends Parent

  def main(args: Array[String]): Unit = 
    println(new Child)
}

With Scala.js 0.6, this would only import from the module rsocket-websocket-client, while Scala.js 1.0.0 will also import from rsocket-websocket-client/RSocketWebSocketClient.

This is a problem because the latter doesn't exist, so bundling won't succeed.

There are a range of reasons why Typescript definitions may end up mentioning module names which don't have a corresponding .js file at bundle-time. For this particular example the third party typings are imprecise.

It's not impossible for me to find a different encoding for re-exporting classes such that this would be safe, but it would end up quite a bit uglier.

Any chance we can revert this behaviour?

[mushtaq]

I want to upvote this in the hope it is given priority.

ScalablyTypes is a key project in making use of existing TS ecosystem. This issue will unblock projects consuming ScalablyTyped to move to Scala.js 1.x.

[gzm0]

Implementation note:

The reason this happens is that when we analyze, we always load the super class, even for native classes which usually do not need this information (I'm unsure whether there is a case it is needed). This is probably a good idea anyways to have a consistent LinkingUnit.

However, when emitting imports, we cannot distinguish between classes that are present because they are used or because merely something declares them in their "metadata". As such we import all of them.

@sjrd the refinement / linking stages could drop the jsNativeLoadSpec if it is not needed. But that in turn would make the IR checker fail / make it impossible for us to check that a native class in principle has a load spec.
Alternatively, we can add relevant analyzer metadata to the LinkedClass. But that would be kind of inconsistent with other refinement.

[sjrd]

I think we should just not generate imports for native JS classes that are not instantiated (i.e., its LinkedClass.hasInstances field is false). We don't call Analyzer#ClassInfo.instantiated() for the superclass of a native JS class (only for the superclass of a non-native JS class, or of course when we use LoadJSConstructor() of the class).

[mushtaq]

Hey, thank you so much @gzm0 for being so responsive!