ScalablyTyped / Converter

Typescript to Scala.js converter
https://scalablytyped.org
GNU General Public License v3.0
221 stars 42 forks source link

sbt plugin seems to remove npmDependencies used in libraryDependencies #293

Closed fdietze closed 3 years ago

fdietze commented 3 years ago

I'm trying to set up a web project with ScalablyTyped and Outwatch. Outwatch is using scalajs-bundler with a snabbdom dependency:

https://github.com/OutWatch/outwatch/blob/master/build.sbt#L142

As soon as I enable the ScalablyTypedConverterPlugin, I get runtime errors in the browser:

/webclient-fastopt-entrypoint.js
Module not found: Error: Can't resolve 'snabbdom' in '/.../web-client/target/scala-2.13/scalajs-bundler/main'

Adding the snabbdom dependency explicitly to my project's npmDependencies solves the problem.

oyvindberg commented 3 years ago

Yes, this is done on purpose. I should probably highlight it more in the docs.

It is done to avoid a circular dependency in sbt tasks. With that scalajs-bundler functionality libraryDependencies help decide npmDependencies, while with ST it's the opposite.

fdietze commented 3 years ago

I see. But that's quite inconvenient. It means adding ScalablyTyped to an existing project may break it silently. And knowing and adding all transtive npm dependencies with its versions could lead to mistakes. Like when using a Scala library (e.g. Outwatch) and it's js dependency (e.g. snabbdom). Every time I update Outwatch, I have to check in the Outwatch sources if the js dependency needs to be updated as well.

Isn't there any way around it?

oyvindberg commented 3 years ago

Well yeah no, unfortunately. We could in theory create a new npmDependenciesST task just for ST and populate npmDependencies with that, but that would also be confusing.

Frankly I think this feature is all kinds of bad and surprising in that it scans the entire runtime class path at build time to pick up magic files and in turn use those to compute the Javascript dependency tree. As such I'm not likely going to work on it.

If you feel like forking the relevant pieces of scalajs-bundler into ST and set up a npmDependenciesST (hopefully with a better name) setting I'll guide you along the way :)

oyvindberg commented 3 years ago

hmm, come to think of it, that would require us to run npm or yarn twice actually, one before sbt computes libraryDependencies and one after, I guess that makes it a non-starter as well. I remember it was pretty hard to wreste sbt into accepting this when developing the plugin

fdietze commented 3 years ago

Well yeah no, unfortunately. We could in theory create a new npmDependenciesST task just for ST and populate npmDependencies with that, but that would also be confusing.

That sounds like a valid solution to me.

I can look into it as soon as the pain is great enough... :smiley:

oyvindberg commented 3 years ago

I'll close this, because I don't intend to work on it. specifying these libraries in npmDependencies will have to be the price of using ST, unfortunately.

cornerman commented 2 years ago

Is there any chance that we can revisit this issue? For me, it becomes a growing pain, when working with multiple libraries that are internally using some js package.

In the scala world with libraryDependencies, all transitive dependencies are automatically included with eviction for conflicting versions.

In the js world with npmDependencies, I now have to mention all of my transitive dependencies, hoping to be in sync with what the corresponding library does. There is no mechanism for version updates/conflicts. I will have to check the other scala project, in order to find out, which version of the npm package should be used.

To me, this only screams for trouble with incompatible versions that will only surface in the runtime. Furthermore, it leads to a lot of noise in our build.sbt. We have to mention many npm packages, eventhough, we are not directly using them. Adidtionally always adding those libraries to stIgnore, because facades do not need to be created.

I would love to help, just would need some more pointers on the mentioned approach.

oyvindberg commented 2 years ago

I personally won't. I'm not saying this is impossible to solve, but it will be hard.

The sbt plugin fundamentally takes npmDependencies and produces libraryDependencies. We cannot also use libraryDependencies to produce npmDependencies. sbt will just hang if you try. I'd be happy to provide further pointers if you want to work on this, but you'll be brought deep inside the belly of sbt.

I also think the pattern of scanning the entire classpath from libraryDependencies to produce npmDependencies is a bad idea. It's surprising, hard to trace, hard to override, and slow.

In the js world with npmDependencies, I now have to mention all of my transitive dependencies, hoping to be in sync with what the corresponding library does. There is no mechanism for version updates/conflicts. I will have to check the other scala project, in order to find out, which version of the npm package should be used.

I'm confused about all this.

How much churn do you have for facade libraries? There is a point in time where you bump such a library. At that point in time you also check which version of the javascript library it now expects, and bump as necessary. And even if you forget, Javascript libraries break compatibility very rarely since people use them without types.

Why do you need to list transitive libraries? npm/yarn will install transitive libraries if you don't mention them? are you not checking in lock files maybe?

workarounds

fdietze commented 2 years ago

Why do you need to list transitive libraries? npm/yarn will install transitive libraries if you don't mention them? are you not checking in lock files maybe?

It's about transitive npm dependencies for direct Scala dependencies. Example: Outwatch (Scala) depends on snabbdom (npm). Snabbdom needs to be listed explicitly in the npmDependencies when using ScalablyTyped. But it's automatically there, when not using ScalablyTyped.

If an outwatch update uses a newer snabbdom version, our build definition would still carry the old version, which could lead to runtime bugs.

oyvindberg commented 2 years ago

Right, you won't need to list all the transitive deps of snabbdom, that was what I was confused about.

How often does this actually happen to you for it to be a problem?

A reminder like this in build.sbt should get you quite far.

val outwatchVersion = "1.0.0" // remember to bump snabbdom when updating this

Given my experience with Scala.js facade libraries, I'd say it is way more frequent that a facade uses an outdated version of the Javascript library and you want to pick a newer one.

cornerman commented 2 years ago

Sorry for the confusion and thanks for clearing that up @fdietze. My explanation was not really good.

@oyvindberg I think that you are right when talking about simple facades, that simply wrap the API. But for example in the case of outwatch, this is not a simple facade, but it just happens to internally use snabbdom for virtual dom diffing. That is an implementation detail, but not the purpose of the scala library. And should not really be a concern of the user of the library.

Now imagine, that you have a scalajs-library using multiple npm packages to provide some functionality. They will all leak into the build.sbt of the library-user.

How much churn do you have for facade libraries? There is a point in time where you bump such a library. At that point in time you also check which version of the javascript library it now expects, and bump as necessary. And even if you forget, Javascript libraries break compatibility very rarely since people use them without types.

In reality, we had to wait for multiple fixes in upstream snabbdom (now having our own fork). That means, that some of our features rely on certain bug-fixes and updates. If you have an outdated version, you will have subtle bugs that will not be catched by any CI. Then, when you use, e.g., scala-steward for automated updates, this will not really work well.