raquo / Airstream

State propagation and event streams with mandatory ownership and no glitches
MIT License
247 stars 28 forks source link

Fix: `SplitByType` macros #133

Closed HollandDM closed 1 week ago

HollandDM commented 1 week ago

This is the continuation of #116 and addresses these issues

HollandDM commented 1 week ago

@raquo the CI survived 100 cases test, I think this should be good enough for your use cases

raquo commented 1 week ago

Thanks! Everything looks good now, I've tested it in the laminar-demo, and it behaves as expected.

I made one small change – handleValue(Foo)(...) now accepts :=> Out by-name instead of a Signal[V] => Out callback, to match Waypoint's collectStatic API. There's not much point in listening to a signal of a single singleton value, at least in the applications I'm imagining, and it's easy enough to just use handleCase for more complex use cases. So I think it's a good tradeoff to avoid the need for _ => for every static page in the URL router.

I still need to do a bunch more work on this, incl. writing docs and ideally a test in Laminar repo, but I plan to release this and some other changes in 17.2 soon.

raquo commented 1 week ago

Yall can try this in 17.2.0-M1 preview. More info in discord

raquo commented 1 week ago

@HollandDM Currently the macros require an import, but I would like to make them available without any special imports.

To make extension methods available without an import, typically I would for example move extension [Self[+_] <: Observable[_], I, O](inline matchSplitObservable: SplitMatchOneObservable[Self, I, O]) methods from SplitOneMacros right into the SplitMatchOneObservable object, to make these methods available on SplitMatchOneObservable type without imports.

I would do the same for extension [Self[+_] <: Observable[_], I](inline observable: BaseObservable[Self, I]) { – move it into the companion object of Observable, where other such implicit conversions are defined. Except I would need to trick Scala 2 into compiling, so uh... probably I would make object Observable extend a new MacroImplicits trait, which would be empty for Scala 2 but would contain this extension method in Scala 3. That's fine I guess... (though if you have better ideas – please share)

My question for you is – would it be ok to move your macro extension methods this way, or is there some reason, whether functional or conventional / stylistic, where this approach would be frowned upon? If it weren't for macros, I would be fine with this, just wanted to check with you if there are any macro-specific considerations for this. I haven't started on this yet.

yurique commented 1 week ago

make object Observable extend a new MacroImplicits trait, which would be empty for Scala 2 but would contain this extension method in Scala 3.

that is how we all do it :)

HollandDM commented 1 week ago

@raquo scala 3 macros tend to be in the same files as the inline def which translated into them. So maybe you will have to move some macro functions as well. I'm not sure what would happen if they are separated, you can try it if you want. About the approach, I think your suggestion is fine. Mose of the big OSS always have some traits like that, named like *PlatformSpecifics (for JS/JVM/Native specific) and *VersionSpecific (for scala 2.12/2.12/3). So I think we can follow the naming convention. Other than that, I'm fine with your restructure.

raquo commented 3 days ago

@HollandDM I made splitMatchOne and splitMatchSeq available without an import (see macro-implicits branch diff here), but I can't figure out how to do the same for the subsequent methods such as handleCase.

In SplitMatchOneMacros, we have both extension methods, and impl methods. The extension methods call into impl methods. What we need is to move or export this:

extension [Self[+_] <: Observable[_], I, O](
  inline matchSplitObservable: SplitMatchOneObservable[Self, I, O]
) { ... }

into object SplitMatchOneObservable. That way, whenever we have a value of type SplitMatchOneObservable, those extension methods will be picked up without any import.

However, I can't figure out how to accomplish that. Naively moving the extension methods and updating impl method references with SplitMatchOneMacros. results in errors like:

[error] -- Error: /Users/raquo/code/scala/airstream/src/main/scala-3/com/raquo/airstream/split/SplitMatchOneObservable.scala:43:6
[error] 43 |      SplitMatchOneMacros.handleCaseImpl('{ matchSplitObservable }, '{ casePf }, '{ handleFn })
[error]    |      ^^^^^^^^^^^^^^^^^^^
[error]    |      Malformed macro parameter
[error]    |
[error]    |      Parameters may only be:
[error]    |       * Quoted parameters or fields
[error]    |       * Literal values of primitive types
[error]    |       * References to `inline val`s

And they're stubborn. I tried various strategies with importing, exporting, extending, but none solved the issue. "Best" I got was when trying to export SplitMatchOneMacros.{*, given} from SplitMatchOneObservable (I was trying to export the extension methods):

[error] 384 |      .handleType[Baz] { (baz, bazSignal) =>
[error]     |                 ^
[error]     |method handleType in object SplitMatchOneObservable does not take parameters

Normally I can sort such things out, but I don't understand macros or their concepts and constraints, so I'm poking at things blindly. Any ideas how to make this work? Would be a bummer if the new feature required additional imports...

HollandDM commented 3 hours ago

@raquo let's me have a look, will contact back to you later