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

Fix to module split-style to include packages for smallmodulesfor #89

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

KristianAN
Copy link
Contributor

@KristianAN KristianAN commented Jun 19, 2024

SmallModulesFor as it was implemented did not include the required list of packages needed for the linker configuration

I was trying to update the js-bridge in bloop-core and I noticed I had messed this up in the last PR... The smallest modules for option requires a list of packages, see: https://github.com/scala-js/scala-js/blob/61e8525eebef3b17290c900c84fea9ba91dc540e/linker-interface/shared/src/main/scala/org/scalajs/linker/interface/ModuleSplitStyle.scala#L43.

So, I'm not sure how you'd structure this ADT in Scala 2, but I think this is an ok solution.

something like this

  val configOne: Config.ModuleSplitStyleJS =
    Config.ModuleSplitStyleJS.SmallModulesFor(List("one", "two"))

  val configTwo: Config.ModuleSplitStyleJS =
    Config.ModuleSplitStyleJS.SmallestModules

  val readConfigOne = jsoniter.readFromString[Config.ModuleSplitStyleJS](
    """{"splitStyle":"SmallModulesFor","packages":["one","two"]}"""
  )

  val readConfigTwo = jsoniter.readFromString[Config.ModuleSplitStyleJS](
    """{"splitStyle":"SmallestModules"}"""
  )
  println(readConfigOne)
  println(readConfigTwo)
  println(jsoniter.writeToString(configOne))
  println(jsoniter.writeToString(configTwo))

prints

SmallModulesFor(List(one, two))
SmallestModules
{"splitStyle":"SmallModulesFor","packages":["one","two"]}
{"splitStyle":"SmallestModules"}

this seems like an reasonable encoding to me.

SmallModulesFor as it was implemented did not include the required list
of packages needed for the linker configuration
val scalaJSModuleKindSplitStyle: Option[ScalaJSModuleKindSplitStyle] = config.moduleSplitStyle.map {
  case ModuleSplitStyleJS.FewestModules => ScalaJSModuleKindSplitStyle.FewestModules
  case ModuleSplitStyleJS.SmallestModules => ScalaJSModuleKindSplitStyle.SmallestModules
  case ModuleSplitStyleJS.SmallModulesFor(packages) =>
    ScalaJSModuleKindSplitStyle.SmallModulesFor(packages)
}
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Could you add some tests to make sure serializing/deserializing works?

config/src/bloop/config/Config.scala Show resolved Hide resolved
@KristianAN
Copy link
Contributor Author

Could you add some tests to make sure serializing/deserializing works?

Will do as soon as I have time!

@KristianAN KristianAN requested a review from tgodzik June 20, 2024 13:24
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks!

@tgodzik tgodzik merged commit 12bc554 into scalacenter:main Jun 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants