From 18ec282b83aee35e768eb4cbdd2514a21807b71a Mon Sep 17 00:00:00 2001 From: David Barri Date: Mon, 30 Aug 2021 11:50:27 +1000 Subject: [PATCH 1/4] Create initial draft of CONTRIBUTING.md --- CONTRIBUTING.md | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..a024856c3 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,124 @@ +Guide to Contributing +===================== + +Contents: + +* Packages +* Files +* Facades +* Non-Facades +* Partially-Supported DOM API +* Binary Compatibility +* Submitting a PR + + +Packages +======== + +TODO: Put `beacon` in `dom` +TODO: Put DOMParser in `dom` with its extra types in its companion object +TODO: Put `gamepad` in `dom` +TODO: Put `push` in `dom`? Everything is `Push*`. Or will there be `Push*` from other APIs too? Do we care? +TODO: Put `sharedworkers` in `dom` +TODO: Put `storage` in `dom`? Does it really warrant its own package? +TODO: Put `webgl/extensions` in `webgl` +TODO: The `ext` package is named confusingly imo. Ideally we could rename it to something clearer. + +``` +org.scalajs.dom — All top-level facades + +-- beacon — Beacon API + +-- crypto — Crypto API + +-- deviceorientation — Device Orientation API + +-- domparser — DOMParser API + +-- ext — Scala helpers for nicer usage + +-- gamepad — Gamepad API + +-- intl — Internationalization API + +-- mediastream — MediaStream API + +-- permissions — Permissions API + +-- push — Push API + +-- raw — deprecated. Remains only for backwards-compatibility + +-- serviceworkers — Service Workers API + +-- sharedworkers — + +-- storage — Storage API + +-- webgl — WebGL API + |   +-- extensions — + +-- webrtc — WebRTC API +``` + + +Files +===== + +* Use `package.scala` for a package object and nothing else. + Also don't include traits/classes/objects. + +* Match the filename to the trait/class/object in it; don't add multiple top-level types. + This is effectively Java-style. + + +Facades +======= + +If a feature has many types that don't share a common unambiguous prefix (like `crypto`), + * create a feature package + * put all of its types in its package + +If a feature has only a few types that don't share a common unambiguous prefix (like `DOMParser`), + * create a facade for the main feature + * create a companion object for the facade + * put all of its types in the companion object + +If a feature has types that all share a common unambiguous prefix (like `Gamepad*`), + * put it all in `dom` + + +Non-Facades +=========== + +* Implicit conversions should go in companion objects so that they are always in scope without the + need for imports. There's no need to group by feature, the types already specify the feature. + +* Add Scala-only utilities that pertain to a specific facade, in the facades companion object + Eg: helper constructors, legal facade values. + +* Add Scala-only utilities that don't pertain to a specific facade, or shouldn't be universally + available (subjective judgement here) in the `ext` package. + + +Partially-Supported DOM API +=========================== + +TODO: Pending discussion in #514 + + +Binary Compatibility +==================== + +Binary compatibility for Scala.JS facades is different than standard Scala. +The following are changes that are indeed incompatible in both formats: + +Don't: + * Remove a trait / class / object + * Change a class into a trait or vice versa + +Here is a non-exhaustive list of changes that would be binary incompatible for Scala classes, but +are compatible for JS facade types: + +You can: + * Remove a member + * Change the type or signature of a member + * Add a field in a trait + * Add an abstract member in a trait + + +Submitting a PR +=============== + +Once you're done making your changes... + +1. Run `sbt prePR` +2. Run `git diff api-reports` and ensure that you aren't breaking backwards-binary-compatibility + (see above). We'll probably automate this step in the future (See #503) +3. Check in and commit the changes to `api-reports` +4. Submit your PR +5. Know that your contribution is appreciated, thank you! From 295113402dfd14fcb697111ff200774adc750fb8 Mon Sep 17 00:00:00 2001 From: David Barri Date: Fri, 3 Sep 2021 10:47:09 +1000 Subject: [PATCH 2/4] Address PR feedback --- CONTRIBUTING.md | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a024856c3..0536b8763 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,14 +15,10 @@ Contents: Packages ======== -TODO: Put `beacon` in `dom` +TODO: Pending discussion in #545 + TODO: Put DOMParser in `dom` with its extra types in its companion object -TODO: Put `gamepad` in `dom` -TODO: Put `push` in `dom`? Everything is `Push*`. Or will there be `Push*` from other APIs too? Do we care? -TODO: Put `sharedworkers` in `dom` -TODO: Put `storage` in `dom`? Does it really warrant its own package? TODO: Put `webgl/extensions` in `webgl` -TODO: The `ext` package is named confusingly imo. Ideally we could rename it to something clearer. ``` org.scalajs.dom — All top-level facades @@ -60,6 +56,8 @@ Facades ======= If a feature has many types that don't share a common unambiguous prefix (like `crypto`), + * firstly, please raise an issue to discuss so that you don't do work only to be asked to undo it + during PR review * create a feature package * put all of its types in its package @@ -81,8 +79,9 @@ Non-Facades * Add Scala-only utilities that pertain to a specific facade, in the facades companion object Eg: helper constructors, legal facade values. -* Add Scala-only utilities that don't pertain to a specific facade, or shouldn't be universally - available (subjective judgement here) in the `ext` package. +* We currently don't see the need for Scala-only utilities that don't pertain to a specific facade, + or shouldn't be universally available (subjective judgement here). + If you believe you've got a compelling use case please raise an issue first to discuss. Partially-Supported DOM API @@ -94,7 +93,7 @@ TODO: Pending discussion in #514 Binary Compatibility ==================== -Binary compatibility for Scala.JS facades is different than standard Scala. +Binary compatibility for Scala.js facades is different than standard Scala. The following are changes that are indeed incompatible in both formats: Don't: @@ -117,8 +116,16 @@ Submitting a PR Once you're done making your changes... 1. Run `sbt prePR` + 2. Run `git diff api-reports` and ensure that you aren't breaking backwards-binary-compatibility - (see above). We'll probably automate this step in the future (See #503) + (see above). The api reports are auto-generated by `prePR` and provide a concise summary of the + entire API. Make sure to look at the top of the file for a description of format. + + Note: We might automate binary-compatibility checking in the future (See #503) but for now, + it's just a helpful tool for reviewing PRs. + 3. Check in and commit the changes to `api-reports` + 4. Submit your PR + 5. Know that your contribution is appreciated, thank you! From eb5ffe5bf19204dde81c218073bf149f925bdccf Mon Sep 17 00:00:00 2001 From: David Barri Date: Sun, 5 Sep 2021 08:20:04 +1000 Subject: [PATCH 3/4] Resolve rules around packages --- CONTRIBUTING.md | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0536b8763..7db6465f3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,31 +15,11 @@ Contents: Packages ======== -TODO: Pending discussion in #545 - -TODO: Put DOMParser in `dom` with its extra types in its companion object -TODO: Put `webgl/extensions` in `webgl` - -``` -org.scalajs.dom — All top-level facades - +-- beacon — Beacon API - +-- crypto — Crypto API - +-- deviceorientation — Device Orientation API - +-- domparser — DOMParser API - +-- ext — Scala helpers for nicer usage - +-- gamepad — Gamepad API - +-- intl — Internationalization API - +-- mediastream — MediaStream API - +-- permissions — Permissions API - +-- push — Push API - +-- raw — deprecated. Remains only for backwards-compatibility - +-- serviceworkers — Service Workers API - +-- sharedworkers — - +-- storage — Storage API - +-- webgl — WebGL API - |   +-- extensions — - +-- webrtc — WebRTC API -``` +In v1.x, there used to be sub-packages grouping some parts of the DOM API by major feature. +In v2.x, we've decided to put everything in `org.scalajs.dom` and get rid of sub-packages. + +The reason for this change is that the real DOM API isn't namespaced in anyway, and the decision +whether to group in a package or not, was subjective and inconsistent. Files From cd080dc1f6d42677e95d0771b5974ca9364d84ae Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Sat, 4 Sep 2021 17:45:26 -0700 Subject: [PATCH 4/4] Add intro, api report ex, other fixes --- CONTRIBUTING.md | 74 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7db6465f3..f1437a618 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,13 +1,24 @@ Guide to Contributing ===================== +Thanks for contributing to scala-js-dom! +We primarily accept PRs for: +* Adding facades for APIs documented in the spec +* Enhancing/fixing existing facades to match the spec +* Adding non-facade Scala utilities to complement specific facades +* Any other bug fixes etc. + +If you would like to make PR that doesn't fall under those categories, +please raise an issue first for discussion so we can give you the go-ahead! + +We look forward to your PRs! + Contents: * Packages * Files * Facades * Non-Facades -* Partially-Supported DOM API * Binary Compatibility * Submitting a PR @@ -35,19 +46,15 @@ Files Facades ======= -If a feature has many types that don't share a common unambiguous prefix (like `crypto`), - * firstly, please raise an issue to discuss so that you don't do work only to be asked to undo it - during PR review - * create a feature package - * put all of its types in its package - -If a feature has only a few types that don't share a common unambiguous prefix (like `DOMParser`), - * create a facade for the main feature - * create a companion object for the facade - * put all of its types in the companion object +We accept facades for any non-deprecated API documented in the spec, including experimental APIs or APIs not supported on all browsers. +* MDN: https://developer.mozilla.org/en-US/docs/Web/API +* WHATWG: https://spec.whatwg.org/ -If a feature has types that all share a common unambiguous prefix (like `Gamepad*`), - * put it all in `dom` +Please: +* Use `def` for read-only properties unless there is a compelling reason it should be a `val` + (i.e., the spec definitively states it is constant) +* Use `Double` for integer-values that can fall outside the range of `Int` +* Add scaladocs via copy-paste from MDN Non-Facades @@ -63,13 +70,6 @@ Non-Facades or shouldn't be universally available (subjective judgement here). If you believe you've got a compelling use case please raise an issue first to discuss. - -Partially-Supported DOM API -=========================== - -TODO: Pending discussion in #514 - - Binary Compatibility ==================== @@ -80,7 +80,7 @@ Don't: * Remove a trait / class / object * Change a class into a trait or vice versa -Here is a non-exhaustive list of changes that would be binary incompatible for Scala classes, but +Here is a non-exhaustive list of changes that would be binary-incompatible for Scala classes, but are compatible for JS facade types: You can: @@ -89,6 +89,32 @@ You can: * Add a field in a trait * Add an abstract member in a trait +To help us enforce binary compatibility, we use API reports. +They are auto-generated by running `prePR` in sbt and provide a concise summary of the entire API. +Note: We might automate binary compatibility checking in the future (see #503) but for now, +it's just a helpful tool for reviewing PRs. + +Here is an example of a binary _compatible_ change in #491 as it appears in the API report diff. +Note that `[JC]` stands for **J**avascript **C**lass, indicating that `HTMLAudioElement` is a facade type and thus this is a compatible change. +```diff +-raw/HTMLAudioElement[JC] def play(): Unit ++raw/HTMLAudioElement[JC] def play(): js.UndefOr[js.Promise[Unit]] +``` + +Here is an example of a binary _incompatible_ change in #458 as it appears in the API report diff. +Even though the `Fetch` object is a facade (a **J**avascript **O**bject), moving it out of the `experimental` package is not binary compatible. +(In this particular case, the change was accepted along with many binary-breaking changes going into 2.0.0.) +```diff +-experimental/Fetch[JO] def fetch(info: RequestInfo, init: RequestInit = null): js.Promise[Response] ++Fetch[JO] def fetch(info: RequestInfo, init: RequestInit = null): js.Promise[Response] +``` + +Anything annotated `[SC]`, `[ST]`, or `[SO]` in an API report is an ordinary Scala class/trait/object, +for which standard binary compatibility rules apply. + +If the above doesn't make sense to you, don't worry! +The majority of useful changes to scala-js-dom are indeed binary compatible. + Submitting a PR =============== @@ -98,11 +124,7 @@ Once you're done making your changes... 1. Run `sbt prePR` 2. Run `git diff api-reports` and ensure that you aren't breaking backwards-binary-compatibility - (see above). The api reports are auto-generated by `prePR` and provide a concise summary of the - entire API. Make sure to look at the top of the file for a description of format. - - Note: We might automate binary-compatibility checking in the future (See #503) but for now, - it's just a helpful tool for reviewing PRs. + (see above). 3. Check in and commit the changes to `api-reports`