angular-ui / ui-grid

UI Grid: an Angular Data Grid
http://ui-grid.info
MIT License
5.39k stars 2.47k forks source link

fix: change $scope.$parent to appScope where applicable #7136

Closed popsail closed 3 years ago

popsail commented 3 years ago

This change prevents an issue where combining a string uiGrid.data and wrapping ui-grid with another component causes data not to be watched and evaluated correctly. Seems to me we'd want to watch the variable on appScope, not necessarily ui-grid's direct parent.

Otherwise, when using a wrapped uigrid, we'll have to pass '$parent.data' instead of 'data', which is a leaky abstraction at best.

commit-lint[bot] commented 3 years ago

Contributors

popsail

Commit-Lint commands
You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR
popsail commented 3 years ago

This change expects appScope to be a $scope. Passing a vm instead will cause it to fail (it not having $watch function).

For backward-compatibility, we can condition using appScope on it having a $watch function, otherwise using $scope.$parent.

popsail commented 3 years ago

Added commit for backward-compatibility - we now condition watching appScope on it being a valid $scope. In other cases (i.e it is a vm) we revert to watching $scope.$parent.

popsail commented 3 years ago

Seems like appScopProvider: vm use cases don't really get fixed by my solution. Will work on another approach and open a new pull/issue.