feathersjs-ecosystem / feathers-vuex

Integration of FeathersJS, Vue, and Nuxt for the artisan developer
https://vuex.feathersjs.com
MIT License
445 stars 108 forks source link

Populated records not updating in realtime #397

Open hybridmuse opened 4 years ago

hybridmuse commented 4 years ago

Are records populated on the feathers server supposed to publish changes to clients in realtime?

Say we have a user with populated contacts as so:

// server (in schema)
User = {
    contacts: [ User ] // populated in hook
}

// client (in model)
setupInstance(data, { models, store }) {
    const { User } = models.api
    if (data.contacts) {
        data.contacts = data.contacts.map(contact => new User(contact))
    }
    return data
}

If one of the populated users were to change its name remotely from its own account, would it be reflected live to the others? Or does the model assignment in setupInstance only serves as a means to save the user back to its own service?

I didn't manage to make the live connection work with populated records. It only works when get/finding the services directly. Please tell me if this is expected behavior.

marshallswain commented 4 years ago

With the way you have it setup, above, if you get a users patched message from the server, it should update the user in the record reactively. However, it won't work automatically for populated data. To be clear, suppose you have this user object come in:

const userFromServer = {
  id: 1,
  email: '1@1.com',
  contacts: [
    { id: 2, email: '2@2.com' },
    { id: 3, email: '3@3.com' },
  ]
}

Since in your setupInstance method you are converting the contacts into User instances, they will go into the users store of your project. If you get a realtime message like users patched for record 2 or 3, the records will update in the store and your UI, reactively.

However if you get a realtime update for record 1, I'm not actually certain what will happen. I'm going to walk through it here.

So I believe it should work either way. You can try setting debugger statements or break points at those locations and see if you find something different.

marshallswain commented 4 years ago

It's important to verify that you're receiving the realtime message from the server, first. In the Network panel under the websocket connection, please make sure you see the realtime event there, first.

hybridmuse commented 4 years ago

Thanks for the detailed walkthrough! The populated records are indeed updated realtime in the store, yet no update on the UI. Here's the relevant Vue code:

// template
<div
    v-for="contact of user.contacts"
    :key="contact._id"
>
    {{ contact.name }}
    <!-- if remote contact changes name, 
    vuex store receives the update yet 
    it's not reflected here on the UI -->
</div>
// setup
const { 
    item: user 
} = useGet({ 
    model: context.root.$FeathersVuex.api.User, 
    id: context.root.$route.params.userId
})

Are populated records in the item from useGet expected to remain in sync with the store too?

hybridmuse commented 4 years ago

Ok, wrote too fast, seems it only gets updated in the store under users but not under the users.contacts relationship. I'll keep investigating.

J3m5 commented 4 years ago

I think this is because the contact item is instantiated with the data that is populated on the current User item and not from the record in the store.

So it can't react from the change in the store because the data is not bound to the store items.

Try to get the matching contact from the store in the setup instance and instantiate it. But I'm not sure that the record will keep it's reactivity, but it's worth trying.

The downside is that it will works only if you already have the record in the store.

J3m5 commented 4 years ago

After looking at the source code, it should works as is because the mergeWithAccessors function is called in order to keep the reactivity in place in the mergeInstance function that is called here when you instantiate the model class.

So it seems to me that reactivity is lost at some point...

hybridmuse commented 4 years ago

Thanks for your input @J3m5. You're right, getting the contacts directly from the store is a solution (although we still have to filter out the main user + other users that might have been independently loaded to the store).

Another workaround would be to fetch the relationships separately from the client, as shown in the guide's example where a patient record first gets loaded, then its appointments.

Still, it seems having the populated records react directly through the nested relationships would be a more elegant and convenient way to do it.

J3m5 commented 4 years ago

We are thinking about a way to populate relationships in the find/get getter with a specific query syntax, that would make it easier and you would only have the relationships populated only when you need it. You could also have the choice to fetch all the data at once or separately.

kshitizshankar commented 4 years ago

@marshallswain @J3m5 Not sure if this was resolved or not - I have the exact same issue. Trying to set up the feathers-graph-populate. The reactivity is maintained when there is a direct mapping

data.user = new User(data.user) Any changes to the user record are reactive.

The reactivity stops when an array of populated data is being mapped. data.tags = data.tags.map(r=> new Tag(r))

Not really sure what's going on...

kshitizshankar commented 4 years ago

Think I found the core issue - It happens because a fastCopy is created for all Objects that are NOT an instance of another model.

// Assign values
    // Do not allow sharing of deeply-nested objects between instances
    // Potentially breaks accessors on nested data. Needs recursion if this is an issue
    let value
    if (_isObject(sourceDesc.value) && !isBaseModelInstance(sourceDesc.value)) {
      value = fastCopy(sourceDesc.value)
    }

https://github.com/feathersjs-ecosystem/feathers-vuex/blob/7b4de7dbdc52919a29c528785aab60d73f0c4e6d/src/utils.ts#L499

So, setting data.user = new Model({}) keeps the reactivity as it becomes an instance of Model, settings data.user = <some other object data> creates a fastCopy and similarly, for data.user = [new Model({})] it creates a fastCopy that removes the reactivity from the models inside the array.

I don't know enough to be able to decide whether it is something that can be safely removed or if the case for an array of Model objects needs to be added in there!

J3m5 commented 4 years ago

What I did previously is, adding a getter on the Model that return the related items with the findGetter. It works nicely and what you have to do is instantiate the items with their Model and then add a getter that call the findGetter with the item ids.

kshitizshankar commented 4 years ago

@J3m5 Could you please share an example?

Not sure what you mean by adding a getter on the Model. Do you mean a vuex getter on the service's store?

The setup instance populates the data in the right Model. After that, if I try to do a find query directly in the setup instance (since we can be sure data exists in the store now) - It still is not reactive.

Another option I explored was to create a custom getter for the service store which takes the ID as input and retrieves the records from the store based on the unpopulated field which contains the list of reference Ids.

  getTags: (state, getters, rootState, rootGetters) => (id) => {
    if (!noteId || !state.keyedById || !state.keyedById[id]) {
      return []
    }
    const tagIds = state.keyedById[id].tagIds
    return (rootGetters['tags/find']({
      query: {
        id: {
          $in: [].concat(tagIds)
        }
      }
    })).data || []
  }

It was hard to manage but worked for me so far (To be honest, I didn't really understand the actual issue where some things were reactive and some things were not - I found that missing keys in instanceDefaults can cause that so tried to fix that and it did solve some of those issues).

The biggest issue for me now, which gets even more highlighted with adding feathers-graph-populate, is that any of the results returned from the server with nested populated data will not maintain reactivity wherever the mapping is 1-to-many (Arrays of Models which can in turn contain an array of models of another service)

So for example -

TaskList: {
    tasks: {
        members: {
        }
    }
}

I'll have to create a getter for TasksLists to retrieve Tasks - but even then, the members records won't be reactive.

How do I solve for reactivity in this nested chain of records?

kshitizshankar commented 4 years ago

Looked at the test cases as well - They only cover reactivity for direct mapping of relational data https://github.com/feathersjs-ecosystem/feathers-vuex/blob/c75b3e457851f434697676a3000b1a0c870d7773/test/service-module/model-relationships.test.ts#L362

For arrays of relational data, it only covers if the data gets added to the store (no test for reactivity) https://github.com/feathersjs-ecosystem/feathers-vuex/blob/c75b3e457851f434697676a3000b1a0c870d7773/test/service-module/model-relationships.test.ts#L438

kshitizshankar commented 4 years ago

@J3m5 Is this what you meant by the getter on the model?

static setupInstance (data, { models, store }) {
....
    //Populate references in store.
    data.tags.map(r=> new Tag(r))

    //Remove the field from data
    delete data.tags

    //Replace the field with a getter to pull records from store using the findGetter
    Object.defineProperty(data, 'tags', {
      get: function () {
        return Tag.findInStore({
          query: {
            id: {
              $in: [].concat(this.tagIds)
            }
          }
        }).data || []
      }
    })

This way, record.tags can be accessed as a property and returns the reactive records. This also hooks nicely to instances where I was querying using the nested property

{
    query: {
        'tags.title': 'mytag'
    }
}

This works but I am not sure if I might be missing something because it says not to perform queries in getters (or is it for that specific case?) https://vuex.feathersjs.com/2.0-major-release.html#don-t-perform-queries-side-effects-in-getters

J3m5 commented 4 years ago

@kshitizshankar This is exactly what I mean ! In the example, it introduce a side effect by fetching the item. The fetching replace the item in the store that trigger the getter and fetch the item again...

kshitizshankar commented 4 years ago

Ohhh.. think I finally understand that example now! I remember trying to do something like that a long time back and there were infinite loops (probably in v1.7 or earlier).

Anyways, this getters approach helps me resolve so many things and not pollute with random service level getters. Thanks a ton! :)

kshitizshankar commented 4 years ago

Created a utility that gets called from setupInstance to push the server populated data to the relevant service store and creates the getter required to maintain nested reactivity. It works nicely with https://github.com/marshallswain/feathers-graph-populate/ and https://github.com/marshallswain/feathers-graph-populate/issues/15

/**
   * This function sets up the server populated data without breaking the reactivity.
   * 1. Adds the incoming populated data in appropriate Feathers-Vuex Service Store
   * 2. Sets up a getter on the populated property key to get reactive records.
   * 2.1. The getter also maintains the order of items based on the unpopulated key.
   *    Eg. tagIds: [1, 2, 3] is used to order the items found by the findGetter when accessing <record>.tags
   *
   * @param {*} data The data from setupInstance
   * @param {*} populatedKey The property key which contains the server populated data and needs to be transformed to return reactive records.
   * @param {*} populateDef Populate Definition Object (Refer to feathers-graph-populate for details)
   * 
   */
  _handleServerPopulatedReferences (data, populatedKey, populateDef) {
    const { model: FeathersVuexModel, keyHere, keyThere = 'id', asArray = false } = populateDef
    if (!FeathersVuexModel) {
      throw new Error('Missing Feathers Vuex Model for the populated relationship', data, populatedKey, populateDef)
    }
    if (!keyHere) {
      throw new Error('Missing unpopulated key for setting up data.')
    }

    // Get Data populated from the server.
    const populatedData = data[populatedKey]

    if (Array.isArray(populatedData)) {
      // Populate multiple records in service store
      populatedData.map(obj => new FeathersVuexModel(obj))

      // delete the populatedKey as it won't be reactive.
      delete data[populatedKey]
    } else {
      // Populate single record in service store
      const modelRecord = new FeathersVuexModel(data[populatedKey])

      // delete the populatedKey as it won't be reactive.
      delete data[populatedKey]
    }

    // We do this irrespective of the populated data.
    // This handles the case where the data is not populated by this record.
    if (asArray) {
      // Setup a findInStore getter on the populatedKey
      Object.defineProperty(data, populatedKey, {
        get: function () {
          const resolved = FeathersVuexModel.findInStore({
            query: {
              [keyThere]: {
                $in: [].concat(this[keyHere])
              }
            }
          }).data || []

          return sortObjectArrayByIdsArray(this[keyHere], resolved, keyThere)
        }
      })
    } else {
      // Setup a getFromStore getter on the populatedKey
      if (keyThere === 'id' || keyThere === '_id') {
        // keyThere is idField
        Object.defineProperty(data, populatedKey, {
          get: function () {
            return FeathersVuexModel.getFromStore(this[keyHere])
          }
        })
      } else {
        // keyThere is some other field
        Object.defineProperty(data, populatedKey, {
          get: function () {
            const resolved = FeathersVuexModel.findInStore({
              query: {
                [keyThere]: {
                  $in: [].concat(this[keyHere])
                }
              }
            }).data || []
            if (Array.isArray(resolved) && resolved.length) {
              return resolved[0]
            }
            return undefined
          }
        })
      }
    }
  }

P.S. Would save a lot of time for someone if we can mention this reactivity issue explicitly in the documentation (https://vuex.feathersjs.com/common-patterns.html#relationships-for-populated-data)

marshallswain commented 4 years ago

@kshitizshankar that looks great! I was just thinking two days ago that it was time to make a utility like this. Care to make a PR to feathers-vuex? This will assist a lot of people in handling populated data, better. If you don't want to make the PR, I can do it tomorrow. Could you share the sortObjectArrayByIdsArray implementation?

If you do choose to make the PR, I think this deserves its own module at src/handle-server-populated-references.ts. The add it to the primary export in src/index.ts, so we can use it like this:

import { handleServerPopulatedReferences } from 'feathers-vuex'

Then we can make docs, accordingly.

kshitizshankar commented 4 years ago

@marshallswain Please go ahead with it if you can.

I still don't know how to make a PR properly... I will definitely get it sorted and hopefully do PRs from next time onwards. Thanks!

Here is the implementation for the sortObjectArrayByIdsArray

import { groupBy, map, isObject } from 'lodash'

/**
 * This function sorts an array of objects based on an array of properties of the objects.
 *
 *
 * @param {Array} propertyValArr Eg: [3,2,1]
 * @param {Array} objectsArr Eg: [{id: 1, value: 'a'}, {id: 2, value: 'b'}, {id: 3, value: 'c'}]
 * @param {String} [propertyKey='id']
 * @returns
 */
export const sortObjectArrayByIdsArray = function (propertyValArr, objectsArr, propertyKey = 'id') {
  if (Array.isArray(propertyValArr) && Array.isArray(objectsArr)) {
    const groups = groupBy(objectsArr, propertyKey)
    const orderedMap = map(propertyValArr, function (i) {
      if (isObject(i)) {
        return undefined
      }
      if (!groups[i]) {
        return undefined
      }
      return groups[i].shift()
    })
    return orderedMap.filter(r => r)
  }
  // Note -
  // In case there is a mismatch and either value is not an array - we return the original objectsArr.
  // This might need to be changed to an empty array based on the desired behaviour
  // Safer option is to send the original value back as this shouldn't actually happen since the propertyValArr is used to find the objectsArr in the first place in our usecase.
  return objectsArr
}

I haven't written any test cases for this, but if it helps - I tried out a few scenarios while I was writing it (Will probably convert to tests later) -

const objectsArr = [{ id: 1, value: 'one' }, { id: 2, value: 'two' }, { id: 3, value: 'three' }, { id: 4, value: 'four' }, { id: 5, value: 'five' }, { id: 6, value: 'six' }, { id: 7, value: 'seven' }, { id: 8, value: 'eight' }]

let propertyValArr

// Case: Invalid Ids mixed with valid Ids - Should ignore the invalid ids and maintain order of valid ones.
propertyValArr = [44, 4, 3, 33, 2, 22, 1, 11]
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr, 'id', true).map(r => r.value)) // Should return objects for 4,3,2,1

// Case: All Invalid Ids - Should return empty array.
propertyValArr = [9]
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr, 'id', true).map(r => r.value)) // Should return []

// Case: Object type values in Ids - Should ignore invalid ids.
propertyValArr = [{ t: 2 }, [[2]], 98, 9, 4, 2]
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr, 'id', true).map(r => r.value)) // Should return objects for 1, 4, 2

// Case: Empty Array  - Should return an empty array.
propertyValArr = []
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr, 'id', true).map(r => r.value)) // Should return empty array

// Case: Not an array  - Should return the original objectsArr.
propertyValArr = null
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr).map(r => r.value)) // Should return all the original objectsArr

// Case: Duplicates - We ignore them.
propertyValArr = [2, 8, 2, 7, 3, 8, 9, 1, 2]
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, objectsArr).map(r => r.value)) // Should return objects for 2,8,7,3,1

// Case: Empty Objects Array
propertyValArr = [2, 8, 2, 7, 3, 8, 9, 1, 2]
console.log(propertyValArr)
console.log(sortObjectArrayByIdsArray(propertyValArr, {})) // Should return empty {}
J3m5 commented 4 years ago

I did something similar 6 months ago.

import _get from "lodash/get";

const getItem = (service, store, select = []) => (id) => {
  const basePath = ["state", service, "keyedById", id];
  const fullPath = basePath.concat(select);
  return _get(store, fullPath);
};

const getFromStore = ({ item, service, select, localField, store }) => {
  const ids = item[localField];
  const itemGetter = getItem(service, store, select);

  return Array.isArray(ids)
    ? ids.map(itemGetter)
    : itemGetter(ids);
};

const findFromStore = ({ item, service, store, find, onlyOne }) => {
  const query = find && (typeof find === "function" ? find(item) : find);
  const { data } = store.getters[`${service}/find`]({ query });
  return onlyOne ? data[0] : data;
};
const generateGetter = ({ from, localField, as, select, find, onlyOne }) => {
  if (!from || !localField || !as) {
    return {};
  }

  const storeGetter = find ? findFromStore : getFromStore;
  return {
    getter: {
      get() {
        return storeGetter({
          item: this,
          service: from,
          select,
          localField,
          store: this.constructor.store,
          find,
          onlyOne,
        });
      },
      enumerable: true,
      configurable: true,
    },
    property: as,
  };
};

export const generateGetters = function(Model) {
  if (Array.isArray(Model.populate)) {
    Model.populate.forEach((lookup) => {
      const { property, getter } = generateGetter(lookup);
      if (property && getter) {
        Object.defineProperty(Model.prototype, property, getter);
      }
    });
  }
};

And I define relttionships in he Model like that:

  static populate = [
    {
      from: "users",
      localField: "userId",
      // foreignField: "_id",
      as: "user",
      onlyOne: true,
    },
    {
      from: "employees",
      localField: "managerId",
      as: "manager",
    },
    {
      from: "companies",
      localField: "companyId",
      as: "company",
    },
    {
      from: "roles",
      localField: "roleIds",
      as: "roles",
      select: "name",
    },
  ];
J3m5 commented 4 years ago

I did another module in order to fetch the items on the client but it's full of ramda functions, I'd need to refactor it.

J3m5 commented 4 years ago

The advantage of doing the joins on the client is that you don't fetch related items two times, you collect the ids, keep the unique ones, remove the ones you already have and then fetch the missing ones. It keeps your backend logic simple as the hooks with authentication and permissions are processed normally. And when the store is populated with the joined items, the getters return them automatically because of the reactivity.

J3m5 commented 4 years ago

If we would be able to fetch the schemas ( as JSON schemas / JSON-API for example), all that could be done automatically.

J3m5 commented 4 years ago

Jsonapi-vuex does the getter trick for relationships with JSONAPI automatically.

fratzinger commented 3 years ago

I've something pretty similar to your two approaches @kshitizshankar, @J3m5. I have this for a year now, it's still improving and it's quite slow for arrays tho. But it's completely reactive. Similar to @J3m5 I have an schema per Model. This schema I use for instanceDefaults as well as setupInstance. Prepare for some pseudocode! It looks something like this:

// assignments.model.js
const props = {
    id: {
      type: Number,
      default: null,
    },
    customer: {
      belongsTo: "Customer",
      default: null,
      secondaryKey: "customerId",
    },
    assignmentSteps: {
      hasMany: "AssignmentStep",
      reactive: true,
      default: () => [],
      // eslint-disable-next-line no-unused-vars
      params: (data, { models, store }) => {
        return {
          query: {
            assignmentId: data.id
          }
        };
      }
    },
    comment: {
      type: String,
      default: "",
    },
    veryImportantDate: {
      type: Date,
      default: () => new Date(),
    },
};

Coming from a sequelize world I called relations: belongsTo and hasMany. I have a BaseModelCustom-class which inherits BaseModelCustom and exposes a static init function, that takes the props. The props default are used in instanceDefaults to create the reactive defaults. If type: Date, then the date will be converted automatically in setupInstance. If a prop is belongsTo or hasMany it will be considered in my init function. I add dynamic getters / setters for each prototype. I also had it in setupInstance, but I think as a prototype definition it's much cleaner and faster. It works perfectly well with mergeAccessors With my example above, every assignment will have the following properties: Object.defineProperties(this.prototype, ...) (this is the respective Model) belongsTo: Customer

The trick with this approach is, it handles the relations completely out of setupInstance (like @J3m5 approach). It's just the 10 lines of code, for the Date-converts:

  static setupInstance(data, { models, store }) {
    const { dates } = this.constructor;

    for (let i = 0, len = dates.length; i < len; i++) {
      const key = dates[i];
      data[key] = data[key] && new Date(data[key]);
    }

    return data;
  }

Some things I think are pretty neat about this:

Some things I don't like with this approach, what's why I've not shared this before:

  1. defining belongsTo and hasMany are not pretty... I like @J3m5 approach, which is similar to feathers-shallow-populate but it just works for relations. I wanted to have a complete schema. I hoped for feathers-schema. That way we could define schemas once for server and client. But I don't see a way to destructure the resolve functions to work with feathers-vuex with (data, { models, store })
  2. The reactive hasMany arrays are pretty slow somehow. That's why I added a reactive: true property, which can be set to false to improve performance by scrapping reactivity.
  3. I have to discard the additionally defined properties before sending to the server with feathers-hooks-common/discard. This one is not too hard, because I keep the keys in a blacklistSaveProperties on each Model.
  4. It has no support for serverAlias yet.

What do you all think about this? I can explain some things further, if you want. I would love to see this baked into feathers-vuex.

What do you all think?