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

Switch property accessors to records-as-objects #2

Open
TheSpyder opened this issue Mar 3, 2021 · 6 comments
Open

Switch property accessors to records-as-objects #2

TheSpyder opened this issue Mar 3, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@TheSpyder
Copy link
Owner

Every property accessor in the entire project is an external. When bs-webapi was created that was the only way to do it. Since then records are compiled to objects so we can use (private) record types to represent the properties.

For example, DomRect:

type t = Dom.domRect;

[@bs.new] external make : (~x: float, ~y: float, ~width: float, ~height: float) => t = "DOMRect"; /* experimental */

[@bs.get] external top : t => float = "";
[@bs.get] external bottom : t => float = "";
[@bs.get] external left : t => float = "";
[@bs.get] external right : t => float = "";
[@bs.get] external height : t => float = "";
[@bs.get] external width : t => float = "";
[@bs.get] external x : t => float = "";
[@bs.get] external y : t => float = "";

Can now be implemented as:

type t =
  pri {
    top: float,
    bottom: float,
    left: float,
    right: float,
    height: float,
    width: float,
    x: float,
    y: float,
  };

[@bs.new] external make: (~x: float, ~y: float, ~width: float, ~height: float) => t = "DOMRect"; /* experimental */

This is still up for debate, because by not using the ReScript-provided abstract types like Dom.domRect this will break compatibility with other DOM-related libraries. The improved ease of use is worth it in property-heavy types like DOMRect, but perhaps it should be used sparingly.

@TheSpyder
Copy link
Owner Author

by not using the ReScript-provided abstract types like Dom.domRect this will break compatibility with other DOM-related libraries

We could perhaps offer a toDomType identity external on all types we do this with to convert the type back to the shared one. This would provide a compatibility option if necessary while we move towards more easy to use record types.

@TheSpyder
Copy link
Owner Author

If this happens we'll need ofDomType as well, integrating with other libraries is a two way street.

The pain of documenting this would be immense; I no longer think it's worth considering for 1.0.

@spocke spocke added the enhancement New feature or request label Jun 3, 2021
@illusionalsagacity
Copy link
Contributor

illusionalsagacity commented Sep 22, 2021

Just occurred to me, what about doing the reverse as a potentially easier migration? e.g.

type t = Dom.domRect;
type internal_t =
  pri {
    top: float,
    bottom: float,
    left: float,
    right: float,
    height: float,
    width: float,
    x: float,
    y: float,
  }

external unwrap: t => internal_t = "%identity"

[@bs.new] external make : (~x: float, ~y: float, ~width: float, ~height: float) => t = "DOMRect"; /* experimental */

[@bs.get] external top : t => float = "";
[@bs.get] external bottom : t => float = "";
[@bs.get] external left : t => float = "";
[@bs.get] external right : t => float = "";
[@bs.get] external height : t => float = "";
[@bs.get] external width : t => float = "";
[@bs.get] external x : t => float = "";
[@bs.get] external y : t => float = "";

@TheSpyder
Copy link
Owner Author

Yeah making custom types not the default reduces the impact, although it does reduce the usability a little, that's an interesting thought 🤔

@illusionalsagacity
Copy link
Contributor

Because of the record improvements in ReScript 11 doing this would also allow up-casting from HtmlDivElement.t to HtmlElement.t for example.

@TheSpyder
Copy link
Owner Author

That is certainly a major advantage - but it depends on breaking away from the compiler types.

It's such a big change, the compatibility issues it would cause mean I'd want to get input from the compiler developers before I invest time in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants