Skip to content

Conversation

@sjrd
Copy link
Member

@sjrd sjrd commented Jan 11, 2021

No description provided.

@sjrd sjrd requested a review from gzm0 January 11, 2021 16:15
@sjrd sjrd force-pushed the scalajs-1.4.0 branch 2 times, most recently from 1db9169 to 81a8ae6 Compare January 11, 2021 17:13
@sjrd
Copy link
Member Author

sjrd commented Jan 12, 2021

Ping @gzm0

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 things.

## Dynamic module loading

In Scala.js 1.3.0, we had introduced [module splitting support]({{ BASE_PATH }}/doc/project/module.html#module-splitting).
Scala.js 1.4.0 goes one step further with dynamic module loading.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth mentioning that dynamic module loading works with any splitting mode and with any number 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.

Good point. I added a sentence at the end of the paragraph.

This happens because `x.getClass()` returns the smallest numeric type that can hold the value `5`, i.e., `java.lang.Byte`.
The previous implementation of `isAssignableFrom` would return `true` for `classOf[j.l.Integer].isAssignableFrom(classOf[j.l.Byte])`, but it now returns `false`.

The above idiom can be fixed by advantageously replacing `isAssignableFrom` by `isInstance`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit thrown off by the usage of "advantageously" here. Would simply omitting it be acceptable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.


As a reminder, libraries compiled with 0.6.x cannot be used with Scala.js 1.x; they must be republished with 1.x first.

## Fixes with compatibility concerns
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the following need mentioning (probably not, just want to point it out):

scala-js/scala-js@0b51d7b
scala-js/scala-js#4212

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so. I've added scala-js/scala-js#4212 to the list of issues at the end, though.

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks for the review :)


As a reminder, libraries compiled with 0.6.x cannot be used with Scala.js 1.x; they must be republished with 1.x first.

## Fixes with compatibility concerns
Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so. I've added scala-js/scala-js#4212 to the list of issues at the end, though.

This happens because `x.getClass()` returns the smallest numeric type that can hold the value `5`, i.e., `java.lang.Byte`.
The previous implementation of `isAssignableFrom` would return `true` for `classOf[j.l.Integer].isAssignableFrom(classOf[j.l.Byte])`, but it now returns `false`.

The above idiom can be fixed by advantageously replacing `isAssignableFrom` by `isInstance`:
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

## Dynamic module loading

In Scala.js 1.3.0, we had introduced [module splitting support]({{ BASE_PATH }}/doc/project/module.html#module-splitting).
Scala.js 1.4.0 goes one step further with dynamic module loading.
Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I added a sentence at the end of the paragraph.

@sjrd sjrd merged commit 66bbaf6 into scala-js:master Jan 12, 2021
@sjrd sjrd deleted the scalajs-1.4.0 branch January 12, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants