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

Enable MiMa #461

Closed
wants to merge 2 commits into from
Closed

Enable MiMa #461

wants to merge 2 commits into from

Conversation

armanbilge
Copy link
Member

This adds MiMa to the build, checking against 1.1.0. A bit silly for v2 of course, but helpful if we want to release another 1.x series.

The only problem found since 1.1.0 was introduced in #427.

My 2 cents: since we are releasing a binary-incompatible v2 anyway, we should freeze binary compatibility in 1.x. Maybe we should make a series/1.x branch (possibly without #427), merge the approved/compatible PRs into there, and then forward merge them into main (aka series/2.x).

@armanbilge armanbilge mentioned this pull request Aug 8, 2021
@sjrd
Copy link
Member

sjrd commented Aug 8, 2021

Usually, I'm all for MiMa, and I enable it in all the repos. But it is really not good at checking JS facade types. It has no idea what is or isn't binary compatible from the point of view of facade types. Since scalajs-dom is almost exclusively filled with facade types, I think enabling MiMa will cause much more pain than benefit.

@armanbilge
Copy link
Member Author

I see, so if I understand correctly, binary compatibility is important here, it's just that MiMa flags up a lot of false positives?

@sjrd
Copy link
Member

sjrd commented Aug 8, 2021

Yes, that's right.

@armanbilge
Copy link
Member Author

Then at least for me, MiMa is still a useful tool for catching bincompat issues even if there will be a lot of false positives ... maybe I'll change my mind after trying it. But if @japgolly isn't worried, then that's fine.

It has no idea what is or isn't binary compatible from the point of view of facade types.

Do you mind expanding on this a little bit i.e. what makes a facade different? I worry there's something here I'm not understanding.

@sjrd
Copy link
Member

sjrd commented Aug 8, 2021

MiMa checks binary compatibility at the class file level, following the JVM linking requirements. For Scala classes, this is a very good model for Scala.js compatibility as well, because sjsir file compatibility is very close to class file compatibility. However, for facades, sjsir compatibility is a lot more permissive. Here is a non-exhaustive list of changes that would be binary incompatible for Scala classes, but that are compatible for JS facade types:

  • Removing a member
  • Changing the type or signature of a member
  • Adding a field in a trait
  • Adding an abstract member in a trait

Most changes in the DOM facades belong to one of those, since it is often fixing types and adding members in classes/traits. They would be flagged as false positives.

The following are changes that are indeed incompatible in both formats:

  • Removing a trait/class/object
  • Changing a class into a trait or vice versa

@armanbilge
Copy link
Member Author

armanbilge commented Aug 8, 2021

Aha, thank you for explaining in detail. That is much more permissive than I realized. I agree, we don't need MiMa here.

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