component / model

Minimalistic extensible data models
122 stars 40 forks source link

refactor http, validation and change events to separate plugins #58

Open ianstormtaylor opened 10 years ago

ianstormtaylor commented 10 years ago

the diff is gonna be a little useless on this one, since i started fresh in a separate repo to begin with. but the goal here is to refactor the core of model to make it completely pluggable. i've move lots of the logic that used to be here into three different plugins:

this has been talked about being done in a #12 #17 #19

note that this is a breaking change, but i think it's a good one, and worth a bit of pain because we end up with a pretty nicely pluggable system that we can build anything on. open to critique in case you think anything in the new version could be improved

@MatthewMueller @TooTallNate @yields @visionmedia @timoxley @jonathanong

ianstormtaylor commented 10 years ago

thinking to myself: could be a strong argument for keeping change events in core, because really what good is a model over an object if there are no change events.

counter argument was that i figured on the backend you never really need change events, and i want those base model to be useful in both scenarios. but even if you are using the model on the backend, you're probably not using a model in scenarios where the performance hit of firing change events is going to matter... so maybe better to just put it in core.

any thoughts on that?

matthewmueller commented 10 years ago

hey @ianstormtaylor this looks great. i don't know if you saw https://github.com/modella/modella, but it's basically a fork of component/model that a few people have been working on. We're trying to achieve the same thing as you're doing with this PR, a completely pluggable model that works on client-side and server-side. We should all collab :-)

ianstormtaylor commented 10 years ago

i checked out modella first, but the reason i submitted this here is cuz i didn't like some of the decisions made to modella since it seems more opinionated than the existing component/model one. i'd rather strip down the core and pull lots of that logic out into plugins (like the http saving logic) but modella seemed like it would be a lot more work to convince on, whereas the issues on model seemed to already be okay with a bunch of these changes. so was mostly a work involved decision from my end

i'm totally down for collabing tho. i think this pull request puts the core close to being feature complete anyways, so wouldn't be much to maintain. i could submit the same stripped down version to modella core if you guys wanted to accept it

the only other thing (completely honestly) that turned me off to modella was that it seems like it has lots of if statements and less standard/consistent coding style, and im an ocd biatch :)

let me know what you think

also, anyone have thoughts on change events in core? probably makes it more extendable too.. that was one issue i was seeing with the .use pattern is that it breaks down more in the OO context instead of a more functional context like in rework

hkjels commented 10 years ago

+1 Nice separation of responsibility there.

bmcmahen commented 10 years ago

+1

matthewmueller commented 10 years ago

@ianstormtaylor yah so we struggled a lot with the "saving" event because it's a weird case to handle (asynchronous event emitter). The reason for this logic is that it simplifies the sync layers a lot. The sync layers simply have to return an array or an object and modella will turn it into a modella instance or collection, emit the appropriate events at the right times, etc.

We also pulled out the ajax sync layer into: https://github.com/modella/ajax

This is definitely worth discussing and something that @rschmukler and I have chatted about a lot, cause it's definitely more opinionated that what you propose. so far though it hasn't really been a problem with any of the sync layers we've implemented including the mongo one (https://github.com/modella/mongo#mquery-support), which adds a ton of additional functionality.

As far as consistency goes, it's the result of 2 authors in different places working on the same code. That's an easy fix though.

We're after the same goal here and i think it'd be silly to maintain 2 nearly identical codebases. I'm totally fine with bringing modella dev into component/model. Modella became a thing in the first place because this PR wasn't getting merged: https://github.com/component/model/pull/30 :-P

I also know @rschmukler is working on a successor to modella that uses generators (on the node side) which gets ride of the need for an async event emitter.

rschmukler commented 10 years ago

@ianstormtaylor @MatthewMueller definitely would like to collaborate. When I first worked on modella (core) I was just starting out on Node, so my style has come quite a ways since then.

As Matt said, I am working on a spiritual successor which uses generators to allow for a lot of hooks/middleware into events (e.g initializing -- which could be hooked into load relations by reference from a database on instantiation, if you wanted) . Hopefully I'll get a working prototype out this weekend and then you guys can help me tear it to bits.

ianstormtaylor commented 10 years ago

i'd rather not use generators i think because i want it to be shared client/server for our models over here. how would that work with generators, you mention using them only on the node side?

@visionmedia whachu think about this pull, slash input on whether change events should be in core

tj commented 10 years ago

doesn't matter to me I don't use it anymore haha, looks good to me. change events in core would probably be nice, definitely gets a point where too much abstraction is just annoying, if you need 6 modules just to make it do something

ianstormtaylor commented 10 years ago

k, i'm planning to merge this this weekend if all goes to plan, just so everyone knows. feel free to add anyone you think might be depending on it

@calvinfo @dominicbarnes @amasad @swatinem @guille @cristiandouce @fengmk2 @johntron @weepy @ForbesLindesay @ericgj @wejendorp :)

ericgj commented 10 years ago

Thx for the heads up. Looks ok to me. I don't use model much now so dont really have an opinion about change events in core vs plugin - as long as the callback interface is the same. I will update my old model plugins, at least model-undoable which seems still worth something.

wejendorp commented 10 years ago

I like it. Nice separation.

basicdays commented 10 years ago

Actually I'd prefer a lot of the transports and what not be out of the core model, as then I can just make a custom transport process on my own (it's enterprisey...). In addition, the primary use I have for this currently is for read only purposes, so all state management wouldn't get used. However, what I would like is better ability to hook into the get/set process. I'm thinking something like having middleware that I could register that would get called on get/set would be awesome, as that would make that process a bit cleaner vs having to event absolutely everything...

With middleware, the component/model-change would only need to register middleware vs the magic that it's doing by keeping old references.

basicdays commented 10 years ago

You know what, I take back my last statement. I'm overengineering things. I'm just going to keep attributes as side-effect free instances, and just handle set events another way.

queckezz commented 10 years ago

I'm using the refactor branch for quite some time now and I really like the changes. Would be awesome if you could merge this since nobody is against it.

cristiandouce commented 10 years ago

:+1:

johntron commented 4 years ago

Somebody want to close this PR?