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

Proposal in Progress: Standardizing events #4656

Open
justinbmeyer opened this issue Dec 6, 2018 · 9 comments
Open

Proposal in Progress: Standardizing events #4656

justinbmeyer opened this issue Dec 6, 2018 · 9 comments
Assignees
Labels

Comments

@justinbmeyer
Copy link
Contributor

Currently, CanJS has too many ways of listening to events. .on, onKeyValue, onEvent, onPatches, etc. Also I'd like to take destructuring into account ...

I'd like to consolidate this and simplify for CanJS 6. Everything will consolidate to an event object:

So instead of:

mAP.on("name", function(ev, newName, oldName){
	console.log(arguments);
});

Everything will be on the event object:

map.on("name", function( {value, oldValue, type} ) { ... })

There will be no more event, newValue, oldValue arguments . This is a breaking change, but I think we can move people to it.

@justinbmeyer justinbmeyer changed the title Proposal: Work in Progress: Standardizing events Proposal in Progress: Standardizing events Dec 6, 2018
@frank-dspeed
Copy link
Contributor

@justinbmeyer can we make please this interface? with subscribe and unsubscribe?

https://github.com/tc39/proposal-observable

@justinbmeyer
Copy link
Contributor Author

@frank-dspeed please create another issue for that. I think something like that can be created on-top of CanJS's symbols APIs, but it can't be the underlying primitive CanJS uses without too many breaking changes.

@justinbmeyer justinbmeyer self-assigned this Apr 6, 2019
@justinbmeyer
Copy link
Contributor Author

Patches look like: {type: "add", key, value}

Property events look like:

{type: "propertyName", oldValue, newValue}

DOM events look like:

{type: 'click', target, currentTarget}

@justinbmeyer
Copy link
Contributor Author

I think patches, dispatched events on objects, and dispatched events on single value observables should all be unified. Here's the ideal:

map.on("keyName", (event) => {
  event //-> {type: "set", key: "keyName", newValue: 1, oldValue: 0, target: map}
} ) // same as canReflect.onEvent(map, "keyName", handler );

obs.on( (event) => {
  event //-> {type: "value", newValue: 1, oldValue: 0, target: obs}
} ) // same as canReflect.onEvent( map, handler );

I think we can get there as follows:

1. Update observables to have new values:

map.on("keyName", (event) => {
  event /* -> {
   type: "keyName", 
   eventType: "set",
   key: "keyName", 
   newValue: 1, 
   oldValue: 0, 
   target: map } */
} ) // same as canReflect.onEvent(map, "keyName", handler );

We could also make .type warn when read.

Then in 6.0 we set type to what it should be long term.

@justinbmeyer
Copy link
Contributor Author

justinbmeyer commented Apr 6, 2019

Instead of eventType, we could use:

  • kind
  • action
  • eventName

@matthewp
Copy link
Contributor

matthewp commented Apr 7, 2019

I like kind here personally

@rjgotten
Copy link

rjgotten commented May 1, 2019

Instead of eventType

Just putting it out there as a possibility; operation is also a good fit and a bit more descriptive as to what the property actually indicates, similar to action.

@justinbmeyer
Copy link
Contributor Author

justinbmeyer commented May 29, 2019

Random notes:

canReflect.onPatches()
canReflect.onEvent(obj, [event], handler)



{
	
}

listenTo 

compute.on(function( {newVal, oldVal} ){

})

Observable 


ObservationRecorder.add(obj) //-> canReflect.onValue() 
ObservationRecorder.add(obj,"event") //-> canReflect.onKeyValue()

value({resolve, lastSet, listenTo}) {
	listenTo("firstName", (ev, newVal,oldVal) => {})

	listenTo(lastSet, (newVal, oldVal) => {})
}


value({resolve, lastSet, listenTo}) {

	listenTo(this, ()=>{
	
	})

	listenTo("firstName", ({newValue}) => {})

	listenTo(lastSet, ({newValue}) => {})
}

¿ what to do with batches ?

@bmomberger-bitovi
Copy link
Contributor

bmomberger-bitovi commented May 31, 2019

Peculiarities about the current events implementation.

  • Events are events, just a notification that something happened. Notifications that a key value changed on an observable were implemented as events.
    • Nothing forces a key value event to have the current key value. You the user can dispatch any key as an event and pass no data.
    • Is there a liability in trusting the event dispatch to inform about the change of data state? It's trivial to trick stache into rendering something not in the view model by dispatching events, for example. https://codepen.io/anon/pen/arQLMP?editors=1111
  • Patches are sort of something different. However, can-event-queue as currently implemented allows you to use the event and dispatch interfaces to hook in by listening to the string "can.patches" as an event name. This is weird.
  • Looking at SimpleMap as an example, it doesn't dispatch patches at all (not on add, set, nor remove of key values). onKeyValue and addEventListener differ only in how the parameters are passed (the latter gets an event object): both fire on dispatching an event.

Some things we talked about on 5/29:

  • onPatches and onEvent probably represent the minimum API surface, as patches operate somewhat different from events.
  • A particular motivating case is that we want to unify how listenTo() works in a value behavior, as currently listening to other properties gets an event in position 0 while listening to the lastSet value does not (it gets newValue in position 0).
    • A better way would be for listening to another property, the last set, or even the whole object to give just one similarly shaped object to the callback.
  • In all cases, event callbacks will be passed only an object which can be destructured. This means that irrelevant values (event object, old value, etc.) don't have to occupy positional parameters and all listeners can just receive an object.
  • Making observable map-likes also be observable value-likes (implementing onValue) is probably good. onValue callbacks would receive patch objects, but individually (one patch per call).
  • Batching patches is good for performance, but the interfaces we're thinking about here for key and value events couldn't support batches if the shape of the callback object only has one action/value. This is why onPatches still has to exist.

More thoughts later.

matthewp pushed a commit to canjs/can-define that referenced this issue Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants