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

Get rid of macro cast in Value. #77

Open
back2dos opened this issue Aug 13, 2021 · 0 comments
Open

Get rid of macro cast in Value. #77

back2dos opened this issue Aug 13, 2021 · 0 comments
Milestone

Comments

@back2dos
Copy link
Member

Currently, there's a macro based transform used for Value that was originally primarily intended for @:external, so that one may write the following (example from readme):

var compass:Compass = ...;
var log:ChipLog = ...;
var movement = new Movement({
  heading: compass.degrees / 180 * Math.PI,
  speed: log.knots * KNOTS_IN_METERS_PER_SECOND,
});

This is cute and all, but since it relies on a @:from macro, it has a number of disadvantages:

  1. top down inference will not work
  2. when having conditionals, they are not observed, i.e. having heading: if (settings.useCompass) compass.degrees / 180 * Math.PI else someOtherMethod is expected to become something like heading: Observable.auto(() -> if (settings.useCompass) compass.degrees / 180 * Math.PI else someOtherMethod) but in reality it becomes heading: if (settings.useCompass) Observable.auto(() -> compass.degrees / 180 * Math.PI) else Observable.auto(() -> someOtherMethod), because top down inference enters into each branch before applying the @:from macro

This was all designed with Haxe 3 in mind and today, I'd consider this a suitable alternative:

var compass:Compass = ...;
var log:ChipLog = ...;
var movement = new Movement({
  heading: () -> compass.degrees / 180 * Math.PI,
  speed: () -> log.knots * KNOTS_IN_METERS_PER_SECOND,
});

There are two ways of making this work:

  1. Remove (at first deprecate) the @:from macro and add a @:from ()->T cast to Value (or perhaps even to Observable for that matter). This will solve problem 2, but not the lack of top down inference
  2. Make the constructor consume a ()->T and wrap that in an auto observable internally. This solves both issues. The breaking change would easily be avoided with an implicit cast (and a deprecated @:from macro that makes code like the above compile), but there's a relative performance overhead from wrapping an observable into an auto observable.

I'm inclined towards option 2, because the overhead won't matter most of the time. Its significance should be rare enough to let the user handle it somehow (worst case by making a class that is not a model, but passes the checker).

In any case I wish to deprecate the direct expression -> auto observable syntax, because it may produce code that doesn't work as intended. If anyone is strongly opposed to that, now is the time to be vocal about it.

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

No branches or pull requests

1 participant