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

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

Open
gr2m opened this issue Feb 23, 2016 · 9 comments

Comments

@gr2m
Copy link
Member

gr2m commented Feb 23, 2016

✨ This is a dreamcode question

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

@gr2m gr2m changed the title Question: how to add .createdAt property to matically to all documents created with hoodie.store.add? Question: how to add .createdBy property to automagically to all documents created with hoodie.store.add? Feb 23, 2016
@gr2m gr2m changed the title Question: how to add .createdBy property to automagically to all documents created with hoodie.store.add? Question: how to add a .createdBy property to automagically to all documents created with hoodie.store.add? Feb 23, 2016
@HipsterBrown
Copy link

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

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

gr2m commented Feb 23, 2016

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

gr2m commented Feb 23, 2016

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

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

@mikehedman
Copy link

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

gr2m commented Feb 23, 2016

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

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

gr2m commented Feb 23, 2016

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

ransomw commented Feb 23, 2016

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:

  • how can hoodie.account.id be added it to each object added by hoodie.store.add as the .createdBy property?
  • should validation be part of the api surface?

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.

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

No branches or pull requests

4 participants