hoodiehq / hoodie-client

:dog: Client API for the Hoodie server
Apache License 2.0
34 stars 25 forks source link

Question: how to add a .createdBy property to automagically to all documents created with hoodie.store.add? #62

Open gr2m opened 8 years ago

gr2m commented 8 years ago

:sparkles: This is a dreamcode question :sparkles:

What would the idea API look like that hoodie-client-store exposes, so that we can read out hoodie.account.id and add it to each object added by hoodie.store.add as the .createdBy property?

Here is a suggestion: we could add a .validate function to hoodie-client-store together with an options.validate argument that can be passed to the constructor, like we have it in hoodie-client-account

That validate function could not only throw custom errors and prevent store.add & store.update, it could also alter data. For example, in our Hoodie Tracker App we only have documents with the properties amount and note. Instead of throwing an error if amount is not a number, we could try to parse if a string was entered, and only if that fails, we would throw an error.

With that in place, we could alter the data by adding a .createdBy property. We could even use that to move the .updatedAt, .createdAt, .deletedAt timestamps that are currently implemented in pouchdb-hoodie-api and instead implement them with the validate function.

So dreamcode for the hoodie.store part of the Hoodie constructor would look like this:

var store = new CustomStore(dbName, {
  validate: function (properties, existingObject) {
    // existingObject is undefined if it’s an `add`. For `.update` and `.remove` it’s set
    // to the properties of the existing object in store

    // directly mutates properties before it’s saved
    properties.createdBy = properties.createdBy || account.id

    // optionally we could allow to return a promise for async validations
  },
  ajax: function () { /* ... */ }
)

Let me know what you think

HipsterBrown commented 8 years ago

From your example, it makes me think that each CustomStore would be a kind of Model for a type of data. So .validate would specifically deal with only one type of object per store, i.e.,

var pencilStore = new CustomStore(dbName, {
  validate: function (properties, existingObject) {
    // validate properties specific to a 'pencil' object
  }
)

var penStore = new CustomStore(dbName, {
  validate: function (properties, existingObject) {
    // validate properties specific to a 'pen' object
  }
)

Otherwise, there might be a bunch of conditionals inside a single .validate function for various forms of validation. Is that how you could see this being used?

mikehedman commented 8 years ago

I'd vote yes on the existence of a prescribed validate function - lots and lots of good things can happen there. To me, that's the bigger issue, so I might re-name/re-task this issue to focus on validate().

Secondary issues...for me, if the purpose of the validate function is to facilitate mutating the record before persistence operations, then I would reconsider use of the word 'validate'. I like validate for the example you gave about changing amount from string to number, but adding additional info and timestamps feels like something different. Maybe instead of 'validate' it could be something like 'beforeSave' (I guess I'm thinking of things like SQL's BEFORE INSERT/UPDATE/DELETE triggers).

This issue talks about hoodie-client automagically adding the createdBy, but it would be interesting to see an example of how the hoodie automagic implementation of validate() works if the dev also wants to specify their own validate function (without overwriting hoodie's).

gr2m commented 8 years ago

it would be interesting to see an example of how the hoodie automagic implementation of validate() works if the dev also wants to specify their own validate function (without overwriting hoodie's).

I would suggest that Hoodie has its internal default validation method that does the timestamps & createdBy. If options.store.validate is set to the Hoodie Client Constructor, we could merge the two internally, running the user-defined one first, then Hoodie’s internal one

gr2m commented 8 years ago

We had a somewhat similar discussion for an API that allows async hooks for before / after signin & signout here: https://github.com/hoodiehq/hoodie-client-account/issues/65

We could do the same thing here and add before:change & after:change events with options.hooks that could mutate

mikehedman commented 8 years ago

running the user-defined one first, then Hoodie’s internal one

If it were me, I would do the Hoodie one first, and then the user-defined, this would allow the user to override the default implementation if desired for some reason.

gr2m commented 8 years ago

the Hoodie one first, and then the user-defined

I can’t think of a use case right now. What speaks in favor of running Hoodie internal validation last: it would help with data integrity

mikehedman commented 8 years ago

I can’t think of a use case right now. And running Hoodie internal validation last would help with data integrity

A use case that argues for doing the Hoodie ones last could be setting the timestamps. For example, if Hoodie's default processing were called first, setting the .createdAt timestamp, but then the user defined extension goes off and performs a (slow) ajax call as part of its validation, the timestamp might be a couple of seconds off. Well, I suppose one could argue what time should be recorded, when the user indicated that they wanted to save, or when the code finally got around to performing the save ;)

But you are correct, data integrity would be better if Hoodie goes second...but dev flexibility would be better if Hoodie goes first.

gr2m commented 8 years ago

dev flexibility would be better if Hoodie goes first

Yeah I agree. We could run into trouble here down the road. I’d say It’s a problem when it’s a problem, because I can’t think of use cases right now

ransomw commented 8 years ago

to preface: there is, of course, no ideal api -- or another way, "ideal," is a subjective word (unless it refers to a set of elements closed under multiplication ;-).

to re-preface: wow, i type slowly.

internal default validation method

this-ish.

b/c there are two things here:

in the context of the first point, adding additional features to document and maintain feels ... creepy-crawly (in a good 'n bad way). an easy modification to the original idea would be to call the function _validate (or _clean or whatever) and not make it part of the public documentation, not for the pending version, anyway. and...

(deleted things)

actually, these are all knee-jerk responses. i'm really not familiar enough with the collective dream to have a solid opinion...

idk, seems like @mikeheadman has some valid [ba-dum-tish] points: some prior bias is making me think an event handler is a more natural place to set .createdBy.

as a separate issue, validation can indeed be cool. might be worthwhile to check out the django validation api for ideas.