Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

normalize whitespace in PlatformDateDirectiveSpec for Scala.js #376

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

jenshalm
Copy link
Contributor

Unit tests for Scala.js starting failing in recent scala-steward PRs due to a recent change in time formatting in Node 18.13:

nodejs/node#46123

The space character before AM/PM had been replaced by a non-breaking space character (\\u202f).
Since the linked ticket hints at this change potentially getting reverted in the future, this PR normalizes whitespace in the date formatter specs for Scala.js so that it is expected to work with the old and new versions of Node.

@jenshalm jenshalm added this to the 0.19.1 milestone Feb 24, 2023
@armanbilge
Copy link
Member

armanbilge commented Feb 24, 2023

Not exactly related, but I had wanted to mention in light of the other discussion: to reduce the maintenance burden of Scala.js, instead of all this platform-specific code you could consider using scala-java-time to share code with JVM.

https://github.com/cquiroz/scala-java-time

It would possibly be less nice for users, but it's probably overall a win maintainability vs usability tradeoff.

@jenshalm
Copy link
Contributor Author

That might be an interesting option for 1.0. I'm just unclear on what it means for JVM users from looking at their documentation. Do they get an empty jar or one that somehow shadows the JVM classes? And what exactly do you think of when you say "less nice for users"?

@armanbilge
Copy link
Member

I'm just unclear on what it means for JVM users from looking at their documentation. Do they get an empty jar or one that somehow shadows the JVM classes?

It shouldn't mean anything for JVM users. You don't add it to JVM dependencies, and they don't get anything :)

For JS users, that library provides Scala.js implementations of the JDK java.time APIs. They still compile against the JDK classes, but when the Scala.js linker is generating the JavaScript code it is able to fish the necessary implementations out of scala-java-time.


And what exactly do you think of when you say "less nice for users"?

It might increase the size of the JS code generated for their applications. This isn't a big deal for Node.js, but browser applications are more sensitive to this sort of thing. However, it depends on their usage pattern, since Scala.js aggressively dead-code-eliminates unused code from the artifact. Many users also already have scala-java-time in their application for some reason anyway. And lastly the size increase may be insignificant compared to the rest of their app or simply pulling in Laika.

Since you have a demo Scala.js application in the repo here, to get a feel for this we can measure the size before/after making the change.

@jenshalm
Copy link
Contributor Author

I'm just unclear on what it means for JVM users from looking at their documentation. Do they get an empty jar or one that somehow shadows the JVM classes?

It shouldn't mean anything for JVM users. You don't add it to JVM dependencies, and they don't get anything :)

Ah, I was confused by the fact that they mention the JVM artefact explicitly: "The scala-java-time library is currently available for Scala (JVM, version 8 and later)". Is there any use case then for the JVM artefacts they publish?

It might increase the size of the JS code generated for their applications. This isn't a big deal for Node.js, but browser applications are more sensitive to this sort of thing. However, it depends on their usage pattern, since Scala.js aggressively dead-code-eliminates unused code from the artifact. Many users also already have scala-java-time in their application for some reason anyway. And lastly the size increase may be insignificant compared to the rest of their app or simply pulling in Laika.

Since you have a demo Scala.js application in the repo here, to get a feel for this we can measure the size before/after making the change.

That would be an interesting experiment. Btw. I think about removing the demo app for the 1.0 branch, as it requires maintaining a Node build just for the little demo. I consider adding support for rendering the AST to the preview server instead (e.g. by adding /ast to the URL) at which point the demo app would be fairly redundant.

@armanbilge
Copy link
Member

Is there any use case then for the JVM artefacts they publish?

Once upon a time ago maybe you could use that library on pre-JDK8, before java.time was introduced. Also I believe the JVM artifacts are published under a different package name instead of java.time.

as it requires maintaining a Node build just for the little demo

Hmm, let me see what you are doing. Ideally it should not be any more burdensome than maintaining Scala.js support in general.

@armanbilge
Copy link
Member

Oh I see, your demo is written using React? And it's bringing in a lot of JS dependencies. The good news is that none of that is necessary :) if it's okay with you, I can open a PR to re-implement the demo with Calico which is a 100% pure Scala frontend library, then we can drop all that package.json stuff. Or, there are other popular options such as Laminar but that's not pure FP.

@jenshalm
Copy link
Contributor Author

That sounds cool, but I still feel like removing it is the more sensible approach. There isn't any feedback on it, I don't feel people are actually using it and it heavily overlaps with the preview server's functionality, especially after it gets expanded for AST support.

@armanbilge
Copy link
Member

Ok, fair enough! 🙁

@jenshalm jenshalm merged commit fad27dc into main Feb 25, 2023
@jenshalm jenshalm deleted the js-date-formatting branch February 25, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants