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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ianstormtaylor
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

hkjels commented Jan 21, 2014

+1 Nice separation of responsibility there.

@bmcmahen
Copy link

+1

@matthewmueller
Copy link
Member

@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: #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
Copy link

@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
Copy link
Contributor Author

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
Copy link
Member

tj commented Jan 23, 2014

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
Copy link
Contributor Author

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
Copy link
Contributor

ericgj commented Jan 24, 2014

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
Copy link
Contributor

I like it. Nice separation.

@basicdays
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

👍

@johntron
Copy link

Somebody want to close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.