Cleanups, part 1. #458
Cleanups, part 1. #458
Conversation
sjrd
force-pushed
the
cleanups
branch
9 times, most recently
from
c89bfa2 to
b257d5e
Compare
|
I'll give this a proper look tomorrow but a question for now: we'll need an automated migration strategy (be it full on scalafix and just regex magic like I often do with scalajs-react example). Any thoughts on this yet? If not, this is me planting the seed. I'm thinking I'll create an issue and start a discussion tomorrow. We might want to make sure that all PRs that break compatibility with 1.x also include whatever we decide is appropriate for migration. |
|
Also this PR targets Also! I think we should be merged all the outstanding PRs first before a big cleanup. Unfortunately this creates more work for you @sjrd now that you've already started but I think it's pretty unfair to push that burden to people who've raised PRs already. We should be making PRs as easy as possible for casual contributors to encourage more collaboration. |
|
+1 to working on a separate branch. At the very least we should run the workflows on outstanding PRs and/or ping the contributors to see what's realistic to merge. |
So far, all the changes in this PR are actually very compatible. Most things are only moved around, and I left deprecated aliases everywhere. So nothing is really necessary to migrate per se. It's only after if you want to get rid of the deprecations. And even here, in most cases it should just be changing some imports. The only real breaking changes are the removal of |
|
Regarding existing PRs, it's up to you, but:
|
I merged in main and re-ran CI on these, it looks like they all pass CI actually and many seem in decent shape. See also #461 to enable MiMa. |
In general, it's much better to rebase PR branches, then force-push them, instead of merging master into PR branches. Merge commits in PR branches create weird log graphs. Most main Scala repos demand clean PR branches without merge commits. I think it would be good to keep that property. |
|
It's also possible to use "Squash and merge" at the time of merging instead, although it is then important not to forget to use that option instead of "Merge". |
After having contributed to a couple of your projects, I know this is your preferred style, and for the record I do think it's a very good one for the reasons you mentioned. Especially for important projects such as Scala.js itself. But, imo this requirement makes casual contributing much much more difficult, and I agree with @japgolly that we want to do as much as we can to support this here.
Eh, define "most" :) E.g. none of the typelevel projects I've contributed to do this. So this is evidence to me that the merge-way works fine too. |
|
Yeah I meant most scala/* projects. I don't know about other organizations. |
|
Also this seems to be @japgolly's style of working too, e.g. japgolly/scalajs-react#938 |
|
Using the squash-merge button all the time seems like it would be a reasonable compromise to keeping clean histories without inconvenience; IIRC it can be enforced in the repo settings too. In my experience, nobody reviews the individual commits (messages + diffs) as carefully as the entire PR, someone digging through git history 6 months down the line does not care for either merge commits or the individual commits that went into each pull request. Squash merge ensures that a contributor can use whatever merge/rebase workflow they prefer, but the final PR that is actually reviewed and useful is what makes it into the master git history |
Well, to counter that with a data point: in scala/scala and in scala-js/scala-js, as well as most other scala-js/*, we do review individual commits, and they must make integral sense. The composition of what goes into a commit, and what commits make up a PR, are very much taken into account when reviewing. |
|
Regarding the separate branch, the If there is a need for a 1.2.0, we could make another branch 1.x off 22e4a70, which is the last commit before the changes for Scala 3 and scalajs-dom 2.x in general. |
|
I think the question is whether there should be one more 1.x release with some of the open PRs merged in. Personally, I think this would be both an easy and a nice thing to do, for those who cannot hop to 2.x immediately. And secondly, whether any sort of migration support can be baked in as @japgolly suggested, although from @sjrd's comment it seems this probably won't be necessary.
And I'm very very glad you do!! A regression in one of those projects is a very different beast than a regression in a facade, imo. |
|
@japgolly are things settled enough on main that we can re-review and get this effort merged? Or do you still have some things to do before? |
There was a problem hiding this comment.
Looks great to me! Thanks for rebasing @sjrd . I pushed two commits of my own:
- merge
masterin which both brings in the test support, and fixes the merge conflicts - added a test case for
NodeListops
Anyway, there are two issues before we merge:
object rawoldexists but isn't used anywhere. Rename back toraw- Adding
@deprecatedto an object doesn't cascade to its type members. Eg. I can writeval x: css.FontFaceRule = ???without getting a deprecation warning. The annotation will unfortunately, need to be applied to all type members.
We introduce deprecated aliases from `raw.*` to the corresponding elements of `dom.*`. The short aliases in `html.*`, `svg.*`, `webgl.*` and `idb.*` are kept. The aliases in `webworkers.*` are deprecated like `raw.*` since they are not shorter than the original names. The aliases in `css.*` are deprecated because the object `css` now conflicts with the DOM `CSS` global object on case-insensitive file systems. The aliases are not really shorter than the original names anyway.
So that it is always available. We also make the implementation class private.
To match the actual name of what these things represent in the standard.
DOMList is now no more than an interface for things that have a `length` and index properties. We add a Seq view of all DOMLists in its companion object. We make it read-only and covariant, as its implementations are actually not mutable in the DOM standard. We review all its subclasses and subtraits: * Make them classes iff they are exposed and can be used with `instanceof`. Their constructors are always private. Otherwise, make them traits. * Explicitly declare `item()` iff it is supported in the given class/trait (it used to be in `DOMList`, but not all implementations support `item()`). We make `NodeList` parameterized and covariant in the type of node. We deprecate `NodeListOf` in favor of `NodeList`.
Instead of a Scala val delegating to a val in a JS global scope object.
It is defined in the "Common Definitions" of WebIDL.
Its extensions are already provided by default in Element and Document.
Instead of XMLHttpRequest and the Ajax extension. Update the readme to remove references to the "extensions" in the process.
Recommend to use `dom.fetch` instead, whose API is basically just as good as `Ajax` was.
|
Rebased and addressed the review. I integrated the changes of the merge commit directly in the relevant commit during the rebase. |
|
Fantastic, thank you @sjrd !! |
This is a first wave of cleanups:
raw.*todom.*ext.*Seqsupport directly onDOMListthrough its companion objectMapsupport forNamedNodeMapexperimental.*todom.*, most notably thefetchAPI, which is now used in the readme instead ofXMLHttpRequest/ext.Ajax