Skip to content

Events #140

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

Merged
merged 1 commit into from
Apr 14, 2018
Merged

Events #140

merged 1 commit into from
Apr 14, 2018

Conversation

ethul
Copy link
Contributor

@ethul ethul commented Apr 10, 2018

Initial work for updating the representation of events. This is still a work in progress, but putting up what I have so far.

Resolves #85
Resolves #135
Resolves #136

@natefaubion @unclechu @arthurxavierx - Any feedback is most welcome.

Resolves #85
Resolves #135
Resolves #136
@ethul ethul merged commit dcfbad0 into master Apr 14, 2018
@ethul ethul deleted the events branch April 14, 2018 13:11
@natefaubion
Copy link
Contributor

Sorry I missed this. I don't see how this is type safe though. If SyntheticEvent is a raw Record and not an opaque newtype, then it cannot be safe to call the event methods (like preventDefault)? Additionally, some properties like defaultPrevented are mutable (Boolean).

@ethul
Copy link
Contributor Author

ethul commented Apr 14, 2018

Good points. Maybe the alternative to go the type class direction (#136 (comment)) would be safer. I would definitely be open to writing this that way. What do you think?

@natefaubion
Copy link
Contributor

I think row types are a fine fit, but treating it as a record is not safe for two reasons:

  • A SyntheticEvent is not immutable before calling persist due to event pooling
  • Something that is not actually a Record should not be typed as Record. This is often convenient but breaks down when you use any Record operations for updates, merges, etc.

I think you could possibly provide an opaque newtype indexed by fields and provide a general SProxy based getter that works over Eff, with partial applications.

@ethul
Copy link
Contributor Author

ethul commented Apr 14, 2018

Thanks. Along these lines sounds good. I was testing out the following. Is this kind of like what you are suggesting?

foreign import data SyntheticEvent_ :: # Type -> Type

type SyntheticEventRow r
  = ( bubbles :: Boolean
    , cancelable :: Boolean
    , currentTarget :: NativeEventTarget
    , defaultPrevented :: Boolean
    , eventPhase :: Number
    , isTrusted :: Boolean
    , nativeEvent :: NativeEvent
    , target :: NativeEventTarget
    , timeStamp :: Number
    , type :: String
    | r
    )

type SyntheticUIEventRow r
  = ( detail :: Number
    , view :: NativeAbstractView
    | r
    )

get
  :: forall eff l r s a
   . RowCons l a r s
  => IsSymbol l
  => SProxy l
  -> SyntheticEvent_ s
  -> Eff eff a
get l r = unsafeGet (reflectSymbol l) r

foreign import unsafeGet :: forall eff r a. String -> SyntheticEvent_ r -> Eff eff a

type SyntheticEvent'' r = SyntheticEvent_ (SyntheticEventRow r)

type SyntheticUIEvent'' = SyntheticEvent_ (SyntheticUIEventRow (SyntheticEventRow ()))

bubbles :: forall eff r. SyntheticEvent'' r -> Eff eff Boolean
bubbles = get (SProxy :: SProxy "bubbles")

detail :: forall eff. SyntheticUIEvent'' -> Eff eff Number
detail = get (SProxy :: SProxy "detail")

@natefaubion
Copy link
Contributor

Yes, exactly.

@ethul
Copy link
Contributor Author

ethul commented Apr 14, 2018 via email

@ethul ethul mentioned this pull request Apr 15, 2018
ethul added a commit that referenced this pull request Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants