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

PROPOSAL: Warn people if doing mutations in the wrong spot #4513

Open
justinbmeyer opened this issue Oct 4, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@justinbmeyer
Copy link
Contributor

commented Oct 4, 2018

TLDR; Performing mutations in getters is bad. It makes code hard to understand. It can cause infinite loops. It can cause performance issues. Lets warn users when they are doing it and give them suggestions (in the logic guide) on how to rewrite their code.

This was discussed on a recent live stream.

The Problem

When reviewing code, I notice that users will perform mutations within Observations and getters like:

DefineMap.extend("MyType",{
  names: {Default: DefineList}
  get name(lastSet) {
    this.names.push( lastSet );
    return lastSet;
  }
})

This is dangerous code and should be re-written as:

DefineMap.extend("MyType",{
  names: {
    value({listenTo, resolve}){
      var names = resolve( new DefineList() );
      listenTo("name", (ev, newName)=> {
        names.push(newName); // TODO: this mutation is ok, because `names` is "owned" by this resolver
      })
    }
  },
  name: "string"
})

The solution

When we detect an "unsafe" mutation, we can log something like:

MyType{}.name is mutating MyType{}.names in an unsafe way. Please use the value() behavior to rewrite this code to avoid MyType{}.names. It's likely possible to make MyType{}.names update itself when MyType.name changes. To turn this warning off, please add unsafe: true to the name property definition.

Other examples:

// Uses changes in `allAddresses` to return a promise.  
// Sets `_notificationsPromise` so it can be later returned.
  _notificationsPromise: '*',
  get notificationsPromise () {
    const allAddresses = this.allAddresses
    if (allAddresses.isPending) {
      return Observation.ignore(() => {
        return this._notificationsPromise
      })()
    }
    let retval
    if (allAddresses.BTC.length + allAddresses.EQB.length > 0) {
      retval = Notification.getList({
        address: {
          $in: allAddresses.BTC.concat(allAddresses.EQB)
        },
        $sort: {
          createdAt: -1
        }
      })
    } else {
      retval = Promise.resolve([])
    }
    // Sets notificationsPromise:
    Observation.ignore(() => {
      this._notificationsPromise = retval
    })()
    return retval
  },
// updates when clips changes, changes clips
// This very likely will cause infinite loop
filteredClips: {
      get() {
        const clips = this.attr('clips');
        const filteredClips = clips.filter(clip => clip.attr('isFavorite'));
        canBatch.after(() => {
          const _clips = this.attr('clips');
          const _filteredClips = clips.filter(clip => clip.attr('isFavorite') && !clip.attr('video:deleted'));
          if (_filteredClips.length < _clips.length) {
            this.attr('clips', _filteredClips);
          }
        });
        return filteredClips;
      },
    },
 // when supported sources changes, this updates other nested information.
 // all of these should be derived from `supportedSources`
    selectedSourceName: {
      get(val) {
        if(this.attr('supportedSources')) {
          val = 'rio';
          this.attr('source.source', val);
          this.attr('source.sourceInformation', {});

          const selectedSource = this.attr('supportedSources').find(source => {
            return source.name === val;
          });
          selectedSource.sourceInputs.forEach(input => {
            this.attr(`source.sourceInformation.${input.codeName}`, '');
          });
          this.attr('selectedSource', selectedSource);
          return val;
        }
      }
    }

@matthewp matthewp added the proposal label Oct 5, 2018

@justinbmeyer

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

Hopefully add CHECK to can-event-queue .. can-define, can-simple-map, can-map.

CHECK is there a can-observation-recorder recording right now? That's not good.

@chasenlehara

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Another example, assuming items is a DefineList:

get sortedItems() {
  const items = this.items;
  return items.sort(() => {
    //
  });
},

The sort docs make it clear that it’s an in-place sort (just like Array.sort), but if you don’t think about that, it’s easy to end up in an infinite loop and not know why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.