Facades should be able to support all values of ModuleKind #2797
Comments
|
Hi, I’m not sure it would be easy to adapt to scalajs-react but a general pattern to suport both is the following:
package myfacade
@js.native
trait MyFacade extends js.Object {
…
}
package myfacade
package global
@JSGlobal
object MyFacade extends MyFacade
package myfacade
package module
@JSImport("my-facade", …)
object MyFacade extends MyFacadeThen, at use site, users can pick either |
|
@julienrf Unfortunately that approach only works when downstream users are using raw facades directly. In order to provide a nicer dev experience and/or build Scala-based features on top of the facades, the library interfaces with the facades directly and so, has to make a choice about which to use (I mean either This below much more accommodating. What we you think about a scheme like this? // Great! This will work for both ModuleKind.NoModule and ModuleKind.CommonJSModule!
@JSGlobal
@JSImport("my-facade", …)
@js.native
object MyFacade
// This will only work for the script style.
// Maybe compiler could warn that ModuleKind.CommonJSModule won't be supported if a user uses it.
@JSGlobal
@js.native
object MyFacade
// This will only work for the module style.
// Maybe compiler could warn that ModuleKind.NoModule won't be supported if a user uses it.
@JSImport("my-facade", …)
@js.native
object MyFacade
// This could fail compilation with an error stating that at least 1 annotation is required
@js.native
object MyFacade |
|
Note that even with CommonJSModule, a number of things still must be |
|
Why not simply allow // Import in module mode, global otherwise.
@JSGlobal
@JSImport("my-facade", …)
@js.native
object MyFacade
// Always global.
@JSGlobal
@js.native
object MyFacade
// Module only.
@JSImport("my-facade", …)
@js.native
object MyFacade
// Depends on method if it works :)
@JSLoadVia(...)
@js.native
object MyFacade
// All illegal
@js.native
object MyFacade
@JSLoadVia(...)
@JSGlobal
@js.native
object MyFacade
@JSLoadVia(...)
@JSImport(...)
@js.native
object MyFacade |
|
I agree with what @gzm0 said. But if @sjrd is still concerned at potential ambiguity of |
|
@gzm0's proposal looks reasonable. Besides the technical aspects, I'd like to discuss the philosophy behind such a feature. My impression, AFAICT, is that the JavaScript ecosystem is transitioning to a module-first world, where "scripts" are not a thing anymore. React itself seems to basically require webpack to be usable. Should we introduce a new feature now in Scala.js that supports the "old" way of doing things in JavaScript? As libraries in the ecosystem move to being defined only as modules, this feature will become obsolete, but we'll have to continue to support it forever because it's a language feature. It boils down to a few questions:
The second bullet is particularly relevant in the context of Scala, which is often criticized for its complexity. |
|
An alternative would of course to do this in terms of Then we could just have a library that supports the cross loading and fade this library out without compromising the language. I'm not 100% sure such a thing is implementable with reasonable usage sites though. |
|
Also, we should explore non-language solutions to the problem. Could we have a linker configuration that "maps" module names to global variables? For example, I could say: "map the var $i_React = require("react.js")into var $i_React = $g.ReactThis would not affect the language surface, which shifts the balance utility/complexity. It could also be used on libraries whose authors were not interested in supporting the double module/script scenario. Which is quite important, as it relieves authors from testing in two entirely different environments. |
They're not supported at all. There's a proposal for dynamic imports but it's a long way away. In any case, it shouldn't be the default, as it does not allow as much dce between modules. |
I'd feel much more solidarity to you in that regard if browsers also supported JS modules. They don't though. They only support scripts. I don't even see that changing, I see it going to scripts / webassembly, not scripts / JS modules. I totally take your point that a JS dev community are using modules more and more. On the other hand, I've never in my life seen a more fickle community that the JS community. Every year the community as a whole has some new processes, tools and direction that spreads like wildfire. It's really cool that they keep experimenting and improving the status quo - I like that aspect - but I'd be very wary in making a long-term decision upon whatever the current trend of the year is. Coming back to technical debate, I think it's fantastic to support modules but I'm wary about reducing support for the "script" option. Browsers are never going to stop supporting script-style JS. |
They will. The necessary works have been under discussion for some time in the Web committees. It's a matter of details, and time. Nevertheless, my point is not really on direct support by the browser, but rather that every non-trivial browser app is developed in terms of modules, and packed into scripts in a build step. Anyway, what about the idea I exposed of a fully link-time based solution? |
True, but I don't think the modules-first approach is suitable for scala.js projects currently. I added some of my concerns in #2681, so resolving that is going be a prerequisite. |
|
First of all please don't ship 0.6.15 without a solution to this issue. Even if there's just a temporary resolution that will be revisited later, I really don't wan't to push unnecessary build constraints upon my users. They're quite significant.
Should that affect Scala.JS?
It will probably work but it might annoy a lot of people to have to copy-paste custom stuff into their SBT (and hope it doesn't go out of date). Especially when the alternative just allowing some annotations in my library, something I'm willing to do. Really I don't see the harm in allowing multiple annotations. It's completely at odds with allowing users to select |
I don't fully understand this. It's not like 0.6.14 had a solution and we are removing it (unless I'm missing something here). All we are doing is make some of the naming more sane. Feature wise, nothing changes. Also, I would like to note that 0.6.15 is already behind schedule, so personally I'm not very fond of delaying it for a feature we don't even know how exactly it is supposed to work yet. |
As far as I understand (I haven't tried this yet) in 0.6.14 I can have an object with |
No, in 0.6.14 it is not allowed to have both
Adding something that will be revisited later is not something we can do in Scala.js at this point. Every new feature has to keep working and be supported for a very long time. It is much safer not to add something, so that we have the time to think it through, and eventually implement the right thing when we know what to do. |
|
Oh. That's unfortunate.
|
|
Unfortunately not. When we designed I think the only reasonable thing to do at this point is to keep using the global variant, and tell scalajs-bundler users to configure it so that the relevant JS library is made available on the global scope. The idea of my link-time solution is that we can also provide a similar solution for the reverse direction: when the library was designed for modules, but a user wants to use it in script mode. That direction would be both more powerful (dce is still possible for module users) and easier to set up for user (a single line of sbt config). |
|
Thanks for the advice. After a day of learning webpack and experimenting with it in a SJS context, I'm going to take your suggestion pretty much verbatim. I'll keep scalajs-react as it is for now and probably when SJS 1.0 comes out I'll switch the library over to module-style. For one to use scalajs-react with bundler, the following webpack config is required: module: {
rules: [{
test: require.resolve('react'),
use: [{loader: 'expose-loader', options: 'React'}]
}, {
test: require.resolve('react-dom'),
use: [{loader: 'expose-loader', options: 'ReactDOM'}]
}],
},
entry: ['react', 'react-dom'],
} |
|
Just my two cents: A linking time solution is essentially another version of Pushing this out to a linking time solution puts a partial burden of figuring out how the library behaves on the library user. Now all facade writers have to publish a readme with a configuration that everybody has to copy verbatim if they want to use the library in "the other" context. So if we introduce a feature like this, I think we really should make sure that the relevant definitions are in the right place. |
|
Strongly agree with @gzm0 here. Also, where there's a will there's a way 😆 In order to reduce burden imposed on my downstream users I've just added this: japgolly/scalajs-react@0d2afd2 They'll still need to add the
|
sjrd
added
the
enhancement
If the codebase refers to an `@JSImport` entity, such as
@js.native
@jsimport("foo.js", "Bar")
class Bar extends js.Object
the codebase must be linked with `CommonJSModule` (or, in the
future, other `ModuleKind`s supporting modules). Linking without
module support causes a linking error.
This causes facades for JS libraries supporting both module-style
and script-style to be faced with an early choice:
* either support linking with modules and use `@JSImport`, or
* support linking without module support, and use `@JSGlobal` with
whatever global names the JS library uses.
It is however impossible to write a facade library that supports
both use cases for their end users.
This commit addresses this issue by adding an optional "global
fallback" to `@JSImport`. We can now define a facade as follows:
@js.native
@jsimport("foo.js", "Bar", globalFallback = "Foo.Bar")
class Bar extends js.Object
That facade will successfully link both with and without module
support. With module support, it links as if declared like in the
first snippet. Without module support, it links as if declared as
@js.native
@jsglobal("Foo.Bar")
class Bar extends js.Object
Using this global fallback, a facade library can cater both for
users who want to take advantages of modules, and users who want to
stick to the script style.
---
Implementation-wise, this is quite easy to implement. We simply
add a new `JSNativeLoadSpec.ImportWithGlobalFallback`. It wraps
both a `JSNativeLoadSpec.Import` and `Global`. That JS native load
spec goes through the entire pipeline and reaches the emitter as
is. The emitter decides which one to use depending on the
`moduleKind`.
|
After thinking more about this, I have decided that pragmatism needs to win here, and that this deserves to be implemented in the language. I chose a slightly different syntax than suggested above, however. Instead of the suggestion @js.native
@JSImport("some-module.js", "ModuleMember")
@JSGlobal("TheGlobalName")
object MyFacade extends js.Objectthe syntax would be @js.native
@JSImport("some-module.js", "ModuleMember", globalFallback = "TheGlobalName")
object MyFacade extends js.ObjectThis is makes much clearer which one of "import" and "global" wins, when both are possible. Because, as stated above, even when linking with modules, It also has the benefit in terms of specification and implementation that each facade still needs to have exactly 1 "load spec" annotation, i.e., 1 annotation among PR at #2957. |
If the codebase refers to an `@JSImport` entity, such as
@js.native
@jsimport("foo.js", "Bar")
class Bar extends js.Object
the codebase must be linked with `CommonJSModule` (or, in the
future, other `ModuleKind`s supporting modules). Linking without
module support causes a linking error.
This causes facades for JS libraries supporting both module-style
and script-style to be faced with an early choice:
* either support linking with modules and use `@JSImport`, or
* support linking without module support, and use `@JSGlobal` with
whatever global names the JS library uses.
It is however impossible to write a facade library that supports
both use cases for their end users.
This commit addresses this issue by adding an optional "global
fallback" to `@JSImport`. We can now define a facade as follows:
@js.native
@jsimport("foo.js", "Bar", globalFallback = "Foo.Bar")
class Bar extends js.Object
That facade will successfully link both with and without module
support. With module support, it links as if declared like in the
first snippet. Without module support, it links as if declared as
@js.native
@jsglobal("Foo.Bar")
class Bar extends js.Object
Using this global fallback, a facade library can cater both for
users who want to take advantages of modules, and users who want to
stick to the script style.
---
Implementation-wise, this is quite easy to implement. We simply
add a new `JSNativeLoadSpec.ImportWithGlobalFallback`. It wraps
both a `JSNativeLoadSpec.Import` and `Global`. That JS native load
spec goes through the entire pipeline and reaches the emitter as
is. The emitter decides which one to use depending on the
`moduleKind`.
If the codebase refers to an `@JSImport` entity, such as
@js.native
@jsimport("foo.js", "Bar")
class Bar extends js.Object
the codebase must be linked with `CommonJSModule` (or, in the
future, other `ModuleKind`s supporting modules). Linking without
module support causes a linking error.
This causes facades for JS libraries supporting both module-style
and script-style to be faced with an early choice:
* either support linking with modules and use `@JSImport`, or
* support linking without module support, and use `@JSGlobal` with
whatever global names the JS library uses.
It is however impossible to write a facade library that supports
both use cases for their end users.
This commit addresses this issue by adding an optional "global
fallback" to `@JSImport`. We can now define a facade as follows:
@js.native
@jsimport("foo.js", "Bar", globalFallback = "Foo.Bar")
class Bar extends js.Object
That facade will successfully link both with and without module
support. With module support, it links as if declared like in the
first snippet. Without module support, it links as if declared as
@js.native
@jsglobal("Foo.Bar")
class Bar extends js.Object
Using this global fallback, a facade library can cater both for
users who want to take advantages of modules, and users who want to
stick to the script style.
---
Implementation-wise, this is quite easy to implement. We simply
add a new `JSNativeLoadSpec.ImportWithGlobalFallback`. It wraps
both a `JSNativeLoadSpec.Import` and `Global`. That JS native load
spec goes through the entire pipeline and reaches the emitter as
is. The emitter decides which one to use depending on the
`moduleKind`.
If the codebase refers to an `@JSImport` entity, such as
@js.native
@jsimport("foo.js", "Bar")
class Bar extends js.Object
the codebase must be linked with `CommonJSModule` (or, in the
future, other `ModuleKind`s supporting modules). Linking without
module support causes a linking error.
This causes facades for JS libraries supporting both module-style
and script-style to be faced with an early choice:
* either support linking with modules and use `@JSImport`, or
* support linking without module support, and use `@JSGlobal` with
whatever global names the JS library uses.
It is however impossible to write a facade library that supports
both use cases for their end users.
This commit addresses this issue by adding an optional "global
fallback" to `@JSImport`. We can now define a facade as follows:
@js.native
@jsimport("foo.js", "Bar", globalFallback = "Foo.Bar")
class Bar extends js.Object
That facade will successfully link both with and without module
support. With module support, it links as if declared like in the
first snippet. Without module support, it links as if declared as
@js.native
@jsglobal("Foo.Bar")
class Bar extends js.Object
Using this global fallback, a facade library can cater both for
users who want to take advantages of modules, and users who want to
stick to the script style.
---
Implementation-wise, this is quite easy to implement. We simply
add a new `JSNativeLoadSpec.ImportWithGlobalFallback`. It wraps
both a `JSNativeLoadSpec.Import` and `Global`. That JS native load
spec goes through the entire pipeline and reaches the emitter as
is. The emitter decides which one to use depending on the
`moduleKind`.
Fix #2797: Add an optional global fallback to `@JSImport`.
|
Implemented in f4896a5 |
|
Cool, that will work well. Only concern though, will this scale as more module kinds are added? The goal is for a library to provide enough info for their code to work regardless of what module kind their downstream users choose. Do you foresee other modules kinds being added? |
|
Yes well definitely have an ES2015 module kind at some point. But semantically it's the same as what we already offer with CommonJS modules. Only the desugaring is different. In general if we ever have more module kinds, the same thing will apply. They will all be semantically based on ES2015 modules. |
Library authors should be able to write facades in such a way that downstream users can use any
scalaJSModuleKindvalue they want/need.Not realising the above vision wasn't being realised just yet, I became very fearful because such a direction would leave me with only 3 unhappy choices for scalajs-react:
@JSGlobalon my facades and don't allow users to useCommonJSModule.@JSImporton my facades and force users to useCommonJSModuleand probably force them to use scalajs-bundler which will in turn force them to only import deps a certain way and, only output one huge single JS file.I think it'd be better to allow us to annotate our facades with both
@JsGlobaland@JsImport, then downstream at link-time have Scala.JS choose one or the other depending on users'ModuleKindsetting.The text was updated successfully, but these errors were encountered: