Skip to content
This repository has been archived by the owner on Nov 9, 2021. It is now read-only.

Disable deep watcher on $props #42

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

romansp
Copy link
Contributor

@romansp romansp commented Oct 17, 2019

Fixes #27.

This PR tries to mitigate issue by dropping deep: true from watcher and provides another way to handle props updates to be passed correctly to Moveable instance.

When container prop is a Vue component  watcher will have to traverse the whole object and eventually "traps" itself trying to build a dependency tree.
@romansp
Copy link
Contributor Author

romansp commented Oct 17, 2019

@probil This is somewhat work in progress. Not sure what scope of changes you expect in this PR. I really like you approach to deep-watching described here: #27 (comment)

I guess it also implies that component props themselves will be constructed dynamically? But in this case not sure how to handle type-checks. Maybe drop $props altogether and switch to $attrs?

Few tests will also be a cool addition to that.

Should I implement all of it in this PR, or open another one?

@probil
Copy link
Owner

probil commented Oct 19, 2019

Hi @romansp

The scope of this PR is to fix Maximum call stack size exceeded and keep existing functionality in place.

I think you can simplify it a bit for now.

const watchReactiveProp = key => function (newValue) {
  const existingValue = this.moveable[key]; 
  if (existingValue === newValue) return; 
  this.moveable[key] = newValue; 
}

export default {
  // ...
  watch: {
      draggable: watchReactiveProp('draggable'),
      resizable: watchReactiveProp('resizable'),
      scalable: watchReactiveProp('scalable'),
      rotatable: watchReactiveProp('rotatable'),
      warpable: watchReactiveProp('warpable'),
      pinchable: watchReactiveProp('pinchable'),
      origin: watchReactiveProp('origin'),
      throttleDrag: watchReactiveProp('throttleDrag'),
      throttleResize: watchReactiveProp('throttleResize'),
      throttleScale: watchReactiveProp('throttleScale'),
      throttleRotate: watchReactiveProp('throttleRotate'),
      keepRatio: watchReactiveProp('keepRatio'),
  }
}

I'll merge and then we can figure out how to make it more maintainable

@romansp
Copy link
Contributor Author

romansp commented Oct 20, 2019

Okay, I updated PR. Changed to something that's closer to #27 (comment).

@probil
Copy link
Owner

probil commented Oct 20, 2019

Remove [WIP] prefix when it's ready for review & merge

@romansp romansp changed the title [WIP] Disable deep watcher on $props Disable deep watcher on $props Oct 20, 2019
@probil probil self-requested a review October 21, 2019 13:35
});

const watchMoveableProps = () => MOVEABLE_PROPS.reduce((acc, prop) => {
acc[prop] = watchReactiveProp(prop, true);
Copy link
Owner

@probil probil Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need deep here 🤔 There is only one prop that can be an array
But let it be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree, I decided to implement it in this way to accommodate for future upgrade to newer versions of Movable, so we don't have to think about changes in underlying API.

@probil probil merged commit 34abcea into probil:master Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Vue warn]: Error in nextTick: "RangeError: Maximum call stack size exceeded"
2 participants