arunoda / react-komposer

Feed data into React components by composing containers.
MIT License
732 stars 70 forks source link

Get rid of null in onData(null) #111

Closed ElegantSudo closed 7 years ago

ElegantSudo commented 7 years ago

The is first argument, which is almost always defined as null in the onData() function is not used by the majority of developers and only adds confusion to the package.

I've met a couple of developers who seriously want to punch their computer because the null is so unnecessary.

smeijer commented 7 years ago

(error, result) is a convention used by node. So I wouldn't advise stepping away from it.

ElegantSudo commented 7 years ago

@smeijer hm. That's certainly not the standard for libraries that face the average developer. It sounds like more of an upstream standard. Just because something is standard, does not mean that it has to be or should forever remain that way.

Do you use it and can you give me an instance/example where I might find it useful?

That's really what I'm concerned about. I think API design choices that most users feel are redundant should be removed.

smeijer commented 7 years ago

I can't explain it better than this blog: http://fredkschott.com/post/2014/03/understanding-error-first-callbacks-in-node-js/

ElegantSudo commented 7 years ago

@smeijer that's interesting, but that article doesn't even touch on the way that react-komposer is using it.

React composer is having you define a completely separate function to handle errors, as opposed to what the article outlines as you passing in a callback and having an argument error hold the value of an error if their is one, allowing the user to check for errors with an if statement.

Here's the difference:

// react-komposer way
onData(onError("error here"), { data });

// article way
onData(function(error, data) {
  if (error) console.log(error);
  // else do something with data
});

Either you're comparing apples to oranges or I'm not thinking straight.

clayne11 commented 7 years ago

You're not quite following properly @ElegantSudo. The onData function that's passed in is a callback function. You callback to it anytime there's new data. It's a fairly simple API that's entrenched in the API of this library as well as the node ecosystem. It would pretty a painful breaking change to change it and there's no real reason to.

arunoda commented 7 years ago

I agree with both of you guys. But most of the time, we use onData(null, data) and this data is loaded usually from multiple sources. Also we tend to handle the error with the component itself.

I am re-thinking about to change it with 2.x: https://github.com/kadirahq/react-komposer/issues/123

macrozone commented 7 years ago

I like the idea of indicating an error in one of the composers, but the UI-component should decide what to show if there is an error.

Also if there are multiple composers, subsequent composers might react on errors from a previous one.

"Railway oriented programming" might be a good buzzword here: http://fsharpforfunandprofit.com/rop/ (good read, in particular the presentation)

arunoda commented 7 years ago

I though I could get rid of this. But we should follow this. And v2 still have this.