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

[draggable] track draggable item scopes #9153

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Nov 19, 2016

Fixes #9151

The bug described in #9151 is caused by the removal of the angular debug info in #7929. A second quick-fix option is to track the element scopes inside the draggable container with a WeakMap().

#9152 is an alternative solution

@tbragin tbragin added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Nov 19, 2016
@stacey-gammon
Copy link
Contributor

Interesting. Do you know why removing angular debug info broke this? I'm curious why it worked with the debug info. Perhaps the debug code somehow stops the elements scope from being garbage collected? Makes me nervous that there are similar bugs, as yet unfound, that suffer the same problem. That PR is about a month old but if it took 30 days to realize we can't create any visualizations in non-dev mode.... there might be more issues lurking.

What are your thoughts @spalger?

@spalger
Copy link
Contributor Author

spalger commented Nov 21, 2016

This code was previously dependent on the $.scope() function which uses the debug info attached to elements to lookup the scope for elements in angular's internal store. This function just returns nothing when debug info is disabled (unfortunately) which is why this code is now tracking the scopes for child directives in a WeakMap.

I totally agree with the notion that this could indicate other undiscovered issues. I've verified that there isn't any other code (except tests) using the .scope() function and I think the rest can be left to smoke testing/qa

@kobelb
Copy link
Contributor

kobelb commented Nov 21, 2016

Are we polyfilling WeakMap? Support seems to be rather non-existent for IE 11. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap

@spalger
Copy link
Contributor Author

spalger commented Nov 21, 2016

Yep, we use the core-js implementation by virtue of the babel-polyfill

@kobelb
Copy link
Contributor

kobelb commented Nov 21, 2016

LGTM

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Nov 21, 2016

Ahah, I see now in the angular documentation about scope, though they aren't very clear that you shouldn't use it outside of debugging though:

Scopes are attached to the DOM as $scope data property, and can be retrieved for debugging purposes. (It is unlikely that one would need to retrieve scopes in this way inside the application.) 

Thanks for the explanation, lgtm!

@spalger
Copy link
Contributor Author

spalger commented Nov 29, 2016

Backported to 5.1 in #9250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.1.1 v5.2.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error when adding an aggregation in visualize
7 participants