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

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