Skip to content

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Oct 15, 2020

No description provided.

@sjrd sjrd requested a review from gzm0 October 15, 2020 12:57
@sjrd sjrd force-pushed the scalajs-1.3.0 branch 3 times, most recently from 4a4096d to 3a6ad56 Compare October 15, 2020 15:32
scalaJSLinkerConfig ~= (_.withModuleSplitStyle(ModuleSplitStyle.SmallestModules))
{% endhighlight %}

#### For different entry points
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should come first. That use case is entirely usefully satisfied by Scala.js core. The "many small modules" use case is useful only when piped through a bundler down the line.


#### For different entry points

Set the `moduleID` for your top level exports and/or module initializers explicitly.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should add

Suggested change
Set the `moduleID` for your top level exports and/or module initializers explicitly.
Set the `moduleID` for your top level exports and/or module initializers explicitly (a module initializer is basically a main method for the module).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think we spell "top-level exports" with a dash in "top-level", don't we?


### Entry Points

Scala.js generated code has two different kinds of entry points:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be spelled "Scala.js-generated code", with a dash, shouldn't it?

Scala.js generated code has two different kinds of entry points:

* Top level exports: Definitions to be called from external JS code.
* Module initializers: Code that gets executed when a module is imported.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Module initializers: Code that gets executed when a module is imported.
* Module initializers: Code that gets executed when a module is imported (i.e., main methods).

?


This also works for more than two public modules, creating intermediate shared (internal) modules as necessary.

#### `SmallestModules`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to add a sentence saying why this can be useful? (i.e., to speed up incremental bundling if the output is fed to downstream tooling)

@gzm0
Copy link
Contributor

gzm0 commented Oct 15, 2020

Yes to all your comments. I'll review the rest tomorrow morning.

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor stuff.

We are excited to announce the release of Scala.js 1.3.0!

This release brings one of the most awaited features for Scala.js: module splitting support!
It is now possible to split the generated .js file in multiple modules, to optimize download size in multi-page applications or speed up incremental bundling.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

into multiple modules?


* [#4195](https://github.com/scala-js/scala-js/issues/4195) `LinkedHashMap` iteration not empty after `clear`
* [#4188](https://github.com/scala-js/scala-js/issues/4188) Make `js.Promise.then` return `js.Promise` instead of `js.Thenable`
* [#4203](https://github.com/scala-js/scala-js/issues/4203) `Matcher.region not mutating the matcher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an escaping problem here.


New interface definitions in `java.util.function.*`:

* `Consumer`, `BiConsumer`, `Supplier`, `Function`, `BiFunction`, `UnaryOperator`, `BinaryOperator` and `BiPredicate`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer has been present in v0.6.32 / v.1.0.0: scala-js/scala-js@7fd9ebb

Among others, the following bugs have been fixed in 1.3.0:

* [#4195](https://github.com/scala-js/scala-js/issues/4195) `LinkedHashMap` iteration not empty after `clear`
* [#4188](https://github.com/scala-js/scala-js/issues/4188) Make `js.Promise.then` return `js.Promise` instead of `js.Thenable`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a potentially source incompatible change. Shall we point this out?

@sjrd
Copy link
Member Author

sjrd commented Oct 16, 2020

Thanks for the review. I've added two commits, one addressing my review and one addressing yours. If LGTY I'll squash everything together before merging.

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nits.

{% endhighlight %}
First, instead of `fastOptJS` / `fullOptJS`, use `fastLinkJS` / `fullLinkJS`.
The outputs of those commands will be in a subdirectory like `project/target/scala-2.13/project-fastopt/`, instead of as a single file `.../scala-2.13/project-fastopt.js`.
You can then create different entry points or generate as many small modules as possible using the following setups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and/or" to make it clear these compose?

* many internal small modules (~50 for this example), approximately one per class.

Generating many small modules can be useful if the output of Scala.js is further processed by downstream JavaScript bundling tools.
In incremental builds, they will not need to reparse the entire Scala.js-generated .js file, but instead only the small modules that have changed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/reparse/reprocess/?


The result type of `js.Promise.then` was changed from `js.Thenable` to `js.Promise`.
This is unlikely to cause any issue in most cases, since `js.Promise` extends `js.Thenable`.
It might cause compilation error in some rare cases due to type inference, or if you declare a subclass of `js.Promise`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors

Co-authored-by: Tobias Schlatter <tobias@meisch.ch>
@sjrd
Copy link
Member Author

sjrd commented Oct 16, 2020

Only nits.

Fixed and squashed :)

@sjrd sjrd marked this pull request as ready for review October 16, 2020 08:43
@sjrd sjrd merged commit a1a8766 into scala-js:master Oct 16, 2020
@sjrd sjrd deleted the scalajs-1.3.0 branch October 16, 2020 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants