Skip to content

Instantly share code, notes, and snippets.

@ryanflorence
Created June 6, 2015 05:44
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save ryanflorence/02b37afbcfcf6f93f679 to your computer and use it in GitHub Desktop.
Save ryanflorence/02b37afbcfcf6f93f679 to your computer and use it in GitHub Desktop.
function app(interactions) {
return interactions.get('thing', 'click')
.startWith(0)
.scan(-1, (x,y) => x+1)
.map(number =>
h('div', [
h('p', `Counter: ${number}`),
h(`my-element`, {ref: 'thing'})
])
);
}
@ryanflorence
Copy link
Author

Now if I'm moving HTML around I don't have the cascade screwing me up, don't have collisions since they need to be unique, and people know that elements with ref are important for events.

I recognize you could do this with element ids, but refs would be like react refs, scoped only to this function, so I don't need them to be unique across an unknowable amount of views.

@staltz
Copy link

staltz commented Jun 6, 2015

That's actually a good idea. From the "people knowing" perspective, either way you need to establish a convention what means "important for events". You could as well have a convention that ev-foo classNames/ids are important for events.

Also selector-based getters would also be scoped to the current function. That is, they don't look inside child components. It's also scoped from above: only searches for the selector within the container where the VDOM renders to DOM.

Not having collisions might be an anti-feature actually. I might be interested in getting all clicks from a set of elements.

What do you mean with "if I'm moving HTML around I don't have the cascade screwing me up"?

@ryanflorence
Copy link
Author

if I'm moving HTML around I don't have the cascade screwing me up

Maybe I didn't really mean cascade, but this:

.foo > .bar

matches:

<div class="foo"><button class="bar"/></div>

But if I need to change the html to this:

<div class="foo"><div class="bar-wrapper"><button class="bar"/></div></div>

The event selector no longer works but I have no idea I need to do anything else if I'm just messing with the markup.

These are the kinds of things that aren't a problem for the original author in the original 30 minutes of writing the code. But for the other 99% of the code's lifetime, it leads to lots of bugs and eventually a brittle system that nobody dares make any changes to.

@ryanflorence
Copy link
Author

Not having collisions might be an anti-feature actually.

Ah yeah, makes sense. ref needs a different name for this then, but yeah, any elements with that "event name" should get the behavior.

@ryanflorence
Copy link
Author

function app(interactions) {
  return interactions.get('foo', 'click')
    .startWith(0)
    .scan(-1, (x,y) => x+1)
    .map(number => 
      h('div', [
        h('p', `Counter: ${number}`),
        h(`my-element`, {interaction: 'foo''})
      ])
    );
}

@staltz
Copy link

staltz commented Jun 6, 2015

<div class="foo"><div class="bar-wrapper"><button class="bar"/></div></div>

Simple: that's a code smell. Selectors in interactions.get() should only be one className. I'm guessing you didn't read cyclejs/cyclejs#128 properly, I said this there:

The other thing to keep in mind, no matter how many fences you put in your API, developers will find a way of misusing them. I was fortunate to hear Erik Meijer in person recommending: "I don't design an API to make it hard to do the wrong thing, actually I make the correct thing easy and the wrong thing possible, so that you can more easily spot the wrong patterns in code reviews. If we had made it hard to do the wrong thing, we couldn't see the difference between proper usage and usage which is trying to bend the tool."

@ryanflorence
Copy link
Author

Yeah, I read it. Sounds like we just have different goals, mine is always to make the right thing the easiest thing. In this case, the wrong and right thing are equally easy, and therefore require style-guides and defining what is smelly code.

@ryanflorence
Copy link
Author

More importantly, even in an environment where everybody knows and agrees to "only use class names for event handling" it is still completely unclear to the person changing the HTML that a specific css class has anything to do with events and might want to remove it completely because of the new theme being applied to the code.

Its about knowing that "this element I'm editing has events attached to it".

@staltz
Copy link

staltz commented Jun 6, 2015

There is nothing inherently visual about ref: 'foo' which would make it obvious to everyone that it is being used by event handlers. This also needs to be taught/agreed upon, specially if the person changing HTML is a designer, React isn't for them immediately obvious in all regards. A simple convention .js-foobar or .eventful-foobar is enough.

interactions.get() with complex selectors could be made a warning.

@edygar
Copy link

edygar commented Jun 7, 2015

@staltz, so, I bet the code below is pretty obvious about what is happening, why not doing like this so?

const NAMED_EVENT === Symbol();

function app(interactions) {
  return interactions.filter((e)=> e === NAMED_EVENT)
    .startWith(0)
    .scan(-1, (x,y) => x+1)
    .map(number => 
      h('div', [
        h('p', `Counter: ${number}`),
        h(`my-element`, {onClick: ()=> interaction.onNext(NAMED_EVENT) })
      ])
    );
}

@ryanflorence
Copy link
Author

There is nothing inherently visual about ref: 'foo' which would make it obvious to everyone that it is being used by event handlers.

That's why my second code example has interaction not ref.

React isn't for them immediately obvious in all regards

Hmm ... onClick={something} is about the most obvious way to map an event to an element that I can think of. But more importantly it doesn't need to be obvious, people doing some visual design, or just refactoring some HTML, are simply moving the element around, they have no reason to touch the event mapping, so they leave it there. They don't have to know anything at all and the code won't break.

A simple convention .js-foobar or .eventful-foobar is enough.

In my experience these conventions never make it past the first group of developers who decide upon it. Then all these "junior" developers "without a clue" who "didn't rtfm", come in and "make a mess".

className is not about events, and never should have been. Cycle.js looks really, really cool, but this one thing is enough for me to not ever consider it. You talk about code smells: using CSS selectors for anything other than CSS (and usually even then) is a code smell to me.

@benoneal
Copy link

benoneal commented Aug 6, 2015

I'm 100% with @ryanflorence on this one. This was immediately the first and biggest problem I saw when reading through the examples, and the whole reason I even came across this conversation (and the lengthy conversations in the repo issues), because I was sure this had raised alarm bells for others too (it had). This isn't just a matter of changing how we think in functional vs OO patterns: this is bad practice.

Class names have no place as event identifiers. Any CSS selector is going to be hugely problematic, and IDs will be a nightmare (no more re-usable elements, and you better hope the ID you choose isn't being used anywhere else in the app). Conventions can't solve problems, they are hacks to manage problems which shouldn't exist in the first place.

Right now, Cycle has effectively zero scoping of events. Scoping it to the div it was applied to in the DOM means single page apps (the primary target audience of frameworks like Cycle) have all events effectively globally scoped, so namespace collisions in any long-term complex scaling app are inevitable and managing multiple re-usable atomic elements on the same page becomes a huge pain in the ass.

It seems the only solutions to the lack of scoped events and hijacking CSS selectors for events is "use a naming convention". I'm sorry, but that's just shit. These problems need to be solved at a fundamental level.

They are an immediate Big Fucking Red Flag to anyone working on serious apps (not Todo MVC) with large teams. I would love to use Cycle, but would be a clusterfuck until these issues are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment