Fix #115: Add ESModule support #116

mushtaq · 2020-09-21T12:20:25Z

No description provided.

gzm0 · 2020-09-21T12:46:08Z

Thank you for your contribution. Please sign the Scala CLA: https://www.lightbend.com/contribute/cla/scala in order for us to be able to accept your contribution.

gzm0 · 2020-09-21T12:47:18Z

I'm aware this isn't (unfortunately) explicitly written in the readme here, but most (if not all) of the Scala.js main repo contribution guidelines apply.

mushtaq · 2020-09-21T13:23:26Z

@gzm0 I just signed the CLA.

I will go over the guidelines. Will appreciate if you could points a few missing things in the PR to get me started.

gzm0

Please add a test. At least a superficial one examining the code paths, ideally one that actually loads another module. Note that this needs a webserver, so it should probably go in seleniumJSHttpEnvTest.

Please put the commit message in the following form:

Fix #115: Add ESModule support

.gitignore

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

mushtaq · 2020-09-22T02:42:50Z

Thanks for the inputs @gzm0.
Will look into adding a test over the weekend.

mushtaq · 2020-09-22T13:11:33Z

@gzm0 I attempted writing a module import test by mapping JSImport to a CDN entry. Will that be ok?

build.sbt

seleniumJSHttpESModuleEnvTest/src/main/scala/org/scalajs/jsenv/selenium/CamelcaseEsModule.scala

seleniumJSHttpESModuleEnvTest/src/test/scala/org/scalajs/jsenv/selenium/LocationTest.scala

mushtaq · 2020-09-23T01:48:27Z

Done with the version upgrade and ESModule setting on by default.

gzm0

Only minor nits. LG otherwise.

Once done, please put the changes in two commits: one with the version changes, one with the fix for module support.

In the one with the version changes, please put a short explanation into the commit body, why you are not re-using scalaJSVersions. (feel free to reference http://www.scala-js.org/news/2020/07/02/announcing-scalajs-1.1.1/).

build.sbt

seleniumJSHttpEnvTest/src/test/scala/org/scalajs/jsenv/selenium/CamelCaseTest.scala

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

As mentioned in the [ScalaJS 1.1.1 release](http://www.scala-js.org/news/2020/07/02/announcing-scalajs-1.1.1), ScalaJS version in `scalajs-js-envs` should be harcoded instead of deriving it from `scalaJSVersion`. `scalajs-js-envs` is extracted out of the ScalaJS repository and will follow its own publishing cadence.

mushtaq · 2020-09-27T08:23:47Z

I have incorporated all the suggestions so far. Thank you.

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

gzm0 · 2020-09-27T09:36:37Z

@sjrd further comments on this?

sjrd · 2020-09-27T09:55:08Z

I have nothing else to add :)

gzm0 · 2020-09-27T15:41:42Z

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

-    val scriptTags =
-      scripts.map(path => s"<script src='${path.toString}'></script>")
+  private def htmlPage(setupJsPath: Path, input: Seq[Input], materializer: FileMaterializer): String = {
+    val setupJs = makeTag(setupJsPath, "text/javascript", materializer)


Hum, am I misunderstanding this now, or would this copy the setup script twice?

If you are looking for ways to consolidate code and the fact that the setup script is not a Path is problematic, I think it would be OK to add Jimfs as a dependency and make in-memory Paths. There is precedent for this in the Node.js env: https://github.com/scala-js/scala-js-js-envs/blob/v1.1.1/nodejs-env/src/main/scala/org/scalajs/jsenv/nodejs/ComSupport.scala#L247

Done. Let me know if I got it right.

gzm0 · 2020-09-27T17:20:29Z

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

-        val page = m.materialize("scalajsRun.html", htmlPage(allScriptURLs))
-
+        val setupJsPath = JSSetup.setupFile(enableCom)
+        val page = m.materialize("scalajsRun.html", htmlPage(setupJsPath, input, m))


At this point, you can just pass this as additional input:

val fullInput = Input.Script(JSSetup.setupFile(enableCom)) :: input /* snip */ htmlPage(fullInput, m) /* snip */

That'll also simplify htmlPage.

gzm0 · 2020-09-27T17:23:55Z

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/JSSetup.scala

-    """.stripMargin
+  def setupFile(enableCom: Boolean): Path = {
+    Files.write(
+      Jimfs.newFileSystem().getPath("setup.js"),


Scala.js style guide demands that this be indented by 4 spaces (continuation line).

Consider:

val code = { s""" /* snip */ """.stripMargin } Files.write( Jimfs.newFileSystem().getPath("/setup.js"), code.getBytes(StandardCharset.UTF_8))

To avoid having to indent the huge string literal too much.

gzm0 · 2020-09-27T17:25:04Z

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala

       |  </body>
       |</html>
    """.stripMargin
  }

+  private def makeTag(path: Path, tpe: String, materializer: FileMaterializer): String = {
+    val url = materializer.materialize(path)
+    s"<script defer type='$tpe' src='${url.toString}'></script>"


Also just noticed this: You can drop the toString (and hence the braces). It's implied by the s interpolator.

gzm0 · 2020-09-27T19:44:26Z

Thank you!

mushtaq · 2020-09-28T07:23:47Z

@gzm0 Any chance of getting a new release soon?

gzm0 requested changes Sep 21, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala Outdated Show resolved Hide resolved

mushtaq force-pushed the esmodules branch from d9e8104 to aae6d72 Compare September 21, 2020 15:42

mushtaq force-pushed the esmodules branch 2 times, most recently from 60d10b2 to 0c455f6 Compare September 22, 2020 12:40

gzm0 requested changes Sep 22, 2020

View reviewed changes

mushtaq force-pushed the esmodules branch from 0c455f6 to 7499b42 Compare September 23, 2020 00:58

mushtaq force-pushed the esmodules branch from 44cf20d to b9e358e Compare September 23, 2020 01:52

gzm0 requested changes Sep 23, 2020

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

seleniumJSHttpEnvTest/src/test/scala/org/scalajs/jsenv/selenium/CamelCaseTest.scala Outdated Show resolved Hide resolved

sjrd reviewed Sep 23, 2020

View reviewed changes

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala Outdated Show resolved Hide resolved

mushtaq force-pushed the esmodules branch from b9e358e to ddb7615 Compare September 27, 2020 08:13

gzm0 changed the title ~~Add ESModule support~~ Fix #115: Add ESModule support Sep 27, 2020

gzm0 approved these changes Sep 27, 2020

View reviewed changes

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala Outdated Show resolved Hide resolved

seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumRun.scala Outdated Show resolved Hide resolved

mushtaq force-pushed the esmodules branch from ddb7615 to b9efc6d Compare September 27, 2020 12:35

gzm0 reviewed Sep 27, 2020

View reviewed changes

mushtaq force-pushed the esmodules branch from b9efc6d to 0ce99d1 Compare September 27, 2020 16:53

gzm0 requested changes Sep 27, 2020

View reviewed changes

Fix scala-js#115: Add ESModule support

f964525

mushtaq force-pushed the esmodules branch from 0ce99d1 to f964525 Compare September 27, 2020 17:50

gzm0 approved these changes Sep 27, 2020

View reviewed changes

gzm0 merged commit bb39519 into scala-js:master Sep 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #115: Add ESModule support #116

Fix #115: Add ESModule support #116

mushtaq commented Sep 21, 2020 •

edited by gzm0

Loading

gzm0 commented Sep 21, 2020

gzm0 commented Sep 21, 2020

mushtaq commented Sep 21, 2020

gzm0 left a comment

mushtaq commented Sep 22, 2020

mushtaq commented Sep 22, 2020

mushtaq commented Sep 23, 2020

gzm0 left a comment

mushtaq commented Sep 27, 2020

gzm0 commented Sep 27, 2020

sjrd commented Sep 27, 2020

gzm0 Sep 27, 2020

mushtaq Sep 27, 2020

gzm0 Sep 27, 2020

gzm0 Sep 27, 2020

gzm0 Sep 27, 2020

gzm0 commented Sep 27, 2020

mushtaq commented Sep 28, 2020

Fix #115: Add ESModule support #116

Fix #115: Add ESModule support #116

Conversation

mushtaq commented Sep 21, 2020 • edited by gzm0 Loading

gzm0 commented Sep 21, 2020

gzm0 commented Sep 21, 2020

mushtaq commented Sep 21, 2020

gzm0 left a comment

Choose a reason for hiding this comment

mushtaq commented Sep 22, 2020

mushtaq commented Sep 22, 2020

mushtaq commented Sep 23, 2020

gzm0 left a comment

Choose a reason for hiding this comment

mushtaq commented Sep 27, 2020

gzm0 commented Sep 27, 2020

sjrd commented Sep 27, 2020

gzm0 Sep 27, 2020

Choose a reason for hiding this comment

mushtaq Sep 27, 2020

Choose a reason for hiding this comment

gzm0 Sep 27, 2020

Choose a reason for hiding this comment

gzm0 Sep 27, 2020

Choose a reason for hiding this comment

gzm0 Sep 27, 2020

Choose a reason for hiding this comment

gzm0 commented Sep 27, 2020

mushtaq commented Sep 28, 2020

mushtaq commented Sep 21, 2020 •

edited by gzm0

Loading