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

Huge increase in fullOptJS file size #40

Closed
cornerman opened this issue Mar 1, 2018 · 16 comments
Closed

Huge increase in fullOptJS file size #40

cornerman opened this issue Mar 1, 2018 · 16 comments

Comments

@cornerman
Copy link
Contributor

cornerman commented Mar 1, 2018

With version 2.0, we see a huge increase in the js file size. Before the update to scribe our bundled size in fullOptJS was 1.4mb, now with the new version of scribe it is 2.8mb.

I suspect this is due to the usage of scala-java-time and locales. We had similar issue in our own app, which is why we have removed these two packages from our app. Now, those are pulled in by scribe. Can we make this optional?

@darkfrog26
Copy link
Contributor

Yeah, I fear that's probably the case. The problem is that in 2.0 it's heavily utilized for date formatting, which is obviously a standard feature of logging. I'd welcome a PR that removes the necessity for them or offers a more clever solution to this problem.

@darkfrog26
Copy link
Contributor

This was deployed and fixed in 2.2.0

@cornerman
Copy link
Contributor Author

Perfect, thanks a lot for the fast reaction!

@fdietze
Copy link

fdietze commented Aug 7, 2018

@darkfrog26
Scribe still pulls in scala-java-locales indirectly via perfolation. Is it possible to remove this dependency as well?

@darkfrog26
Copy link
Contributor

@fdietze, unfortunately, for any date formatting it's required. If you have another suggestion of how to accomplish that without it, I'd be happy to consider it.

@fdietze
Copy link

fdietze commented Aug 8, 2018

I understand. One could use java.util.Date. While most methods are deprecated, it is available on the jvm and scalajs (I don't know about native). These should be enough for logging.

import java.util.Date

val d = new Date()
val year = d.getYear + 1900
val month = d.getMonth + 1
val day = d.getDate
val hour = d.getHours
val minute = d.getMinutes
val second = d.getSeconds

println(f"$year%04d-$month%02d-$day%02d $hour%02d:$minute%02d:$second%02d")

I suspect that the increased file size prevents many scalajs-projects from using scribe.

@darkfrog26
Copy link
Contributor

@fdietze while I agree, the simple use-case of date formatting would be solved with java.util.Date, there are advanced use-cases that it does not handle. If you can offer a way to implement the methods in CrossDate(https://github.com/outr/perfolation/blob/master/core/shared/src/main/scala/perfolation/CrossDate.scala) without using the dependency I would happily drop it.

While I agree the size is annoying, I find it unlikely it's a big issue for most people as it's incredibly common for initial loading of sites to be several meg. However, I concur that the logging library shouldn't account for a large percentage of it. ;)

@fdietze
Copy link

fdietze commented Aug 8, 2018

I didn't know that you have already implemented CrossDate using js.Date:
https://github.com/outr/perfolation/blob/master/core/js/src/main/scala/perfolation/JavaScriptCrossDate.scala
This is great! I thought it would be using scala-java-locales.

So as I see it now, the only problem seems to be the usage of java.util.Locale:
https://github.com/outr/perfolation/blob/master/core/js/src/main/scala/perfolation/Platform.scala#L8

What is this line exaclty for?

@darkfrog26
Copy link
Contributor

The Scala.js implementation of Locale doesn't properly set the default locale so if you don't set that line a NPE is thrown when you try to access anything.

@cornerman
Copy link
Contributor Author

Hmm, interesting. What effect does the setDefault of the locale have on js-date?What do you mean with "access anything" here?

For us, the file size is really a problem, because we worked hard to avoid this dependency ourselves. It pulls in all locale strings of all languages into the minified/optimized javascript bundle - which really is unnecessary. I would really like to make this optional or remove it, if it possible in any way :)

Furthermore: Can we safely assume a US locale? How would one override this decision?

@darkfrog26 darkfrog26 reopened this Aug 14, 2018
@darkfrog26
Copy link
Contributor

@cornerman and @fdietze, I would be happy to completely remove that functionality. It was intended as placeholder until we could get a better solution. If either or both of you are willing to work on a PR to perfolation or scribe I'd be happy to assist any way I can. I've re-opened this ticket as I agree this is an issue that should be solved if it can be.

@cornerman
Copy link
Contributor Author

Cool! We will try to start working on this soonish :)

@darkfrog26
Copy link
Contributor

@cornerman, awesome. Feel free to message me on Gitter if you need anything from me.

@darkfrog26 darkfrog26 added this to the 2.7.0 milestone Aug 27, 2018
@darkfrog26
Copy link
Contributor

@cornerman and @fdietze, any updates on this?

@cornerman
Copy link
Contributor Author

@darkfrog26 nothing new for now. i currently do not have much time for taking care of this. so, maybe we should remove it from the milestone if it is a blocker? eventually, we will definitely revisit this issue, because the size issue matters for us - just does not hurt enough yet :)

@darkfrog26 darkfrog26 removed this from the 2.7.0 milestone Dec 2, 2018
@darkfrog26
Copy link
Contributor

Though this was resolved in 2.7.2, it was drastically improved in 2.7.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants