Comments (8)
@gtreskas As the author of the referenced fix I'm genuinly baffled about basically every sentence I just read here 😕
Based on the changes here #94 still the issue persists when you use SmartLink control. The complete Properties panel dissapear in case of sap.ui.comp.navpopover.SmartLink.
No, this is a different issue. It happens while executing port.postMessage(detectEvent.detail)
. Circular references are handled by its structured clone algorithm. Therefore having those is not an issue here - except there is actually a bug in this algorithm (sounds quite unlikely, but it is still human written code after all).
I resolved the issue with circular
dependenciesreferences by adjusting _prepareMessage method, setting the current.target[key] = null in case of circular reference is found
This portion of code does not find/handle circular references explicitely, it just re-uses already parsed references - circular or not. And with this, it also resolves circular references properly. So you did not "resolve the issue", you introduced a bug by altering the result.
I dont understand why we should use an existing
dependencyreferences in the first place --> This may be the root of the new circular reference, because it may re-introduce a circulardependencyreference.
Cause the aim is on purpose to keep the data as it is, not getting rid of any references. Resolving circular dependencies references does not mean removing them but handle them properly while removing methods from the structure (which is btw the one and only reason why the message is parsed in the first place).
Once I did the above, the circular dependency disappeared, but I was playing around with a Fiori Application Smart Template List Report and the performance became a disaster. The _prepareMessage is very slow and makes the whole experience very bad.
Some simple measurements confirm my suspicion: The _prepareMessage
implementation is actually faster than the previous used JSON.parse(JSON.stringify(object))
coding. Do you have other results?
The only "disaster" in performance I did notice, is when clicking on a sap.ui.comp.navpopover.SmartLink
node in the control-tree and its message is parsed async in background. I don't know what is wrong with the sap.ui.comp.navpopover.SmartLink
control, but the retrieved data for it is HUGE (which is still not the root cause here but responsible for the bad perceived performance when further navigating within the control tree).
Of course, this is performance wise worse than a directly failing JSON.stringify
. I really hope nobody is surprised here: Properly parsing an object is slower than not parsing an object at all. (Konfuzius) 🙈
I would suggest adding an extra parameter called "level" (like the node.js util.inspect method) where you can determine in what kind of depth should the reading of properties stop. I think is a waste of time reading the complete depth, especially in aggregation, association object, where the depth can become very deep to handle.
The data that is parsed is not only the control/model information, but also e.g. the control tree. For each hierarchy level, the datastructure requires 2 "levels" within the provided message (plus some additional fixed structural overhead). So a control tree of e.g. depth 20 requires a minimum(!) of "level" 40 to be parsed in _prepareMessage
. The issue with sap.ui.comp.navpopover.SmartLink
happens already with a max-level of 6 (in words: "six").
Even though an "infinite" depth parsing of the control- or model-data is rarely (to never) required, it would not resolve this particular issue, and would furthermore only be useful in the same amount of rare (to none) cases. And it could cause missing information e.g. in the control tree.
Let me know if I can help you with something!
Lets find the actual root cause, then we can make informed decisions based on it.
And for that, I would start at where the issue actually occurs: The postMessage
method of the chrome.runtime Port - what are its limitations, what are potential faulty inputs, what are known issues, checking bug reports etc.
And from there, check the actual input of the call in case serializing sap.ui.comp.navpopover.SmartLink
control information. As mentioned, the input can be stripped down to depth level 6 in the _prepareMessage
method.
THE END
from ui5-inspector.
@SeeSharpSoft : Thanks for the detail explanation, well it may be I was working too late yesterday and I miss to state the facts in the right way, sorry don't get offended or something from the issue here, I m sure you and your team are doing great job here.
So lets just go to the concrete case and try to reproduce the issue and I can show you on my screen how you can reproduce the issue or setup a call and have a look together.
The actual root cause is a circular reference and there was a comment on your code,, which was stating that you are handling it through copying an existing structure, but you are right, maybe I missunderstood your code. Currently I build a small fallback method in my fork which doesnt resolve the problem directly, but at least it works for now.
Let me know concretaly how to proceed?
Best Regards
from ui5-inspector.
@gtreskas Just to clarify: I am a returning, but still individual contributor to this project. Not part of the team, not having any more saying in this than you.
My lengthy response is about making informed decisions based on a proper root cause analysis, instead of trying to patch its effects. Those patches often causes additional issues that are then patched again etc. I am an advocate of the saying: "If you have one hour to solve a problem, then spent 50min on the problem and 10min on the solution, not vice versa!"
Don't get me wrong: I do acknowledge the issue and can reproduce it fairly easy on the sap.ui.comp.navpopover.SmartLink
control - but I myself didn't figure its actual root cause yet. I do not know the problem well enough to start talking about a solution.
The actual root cause is a circular reference
If you found the root cause - awesome!
So I assume you are refering to a specific circular reference then (as in general, circular references shouldn't be an issue). So here are my thoughts:
- Why is this particular circular reference an issue while others aren't?
- And with respect to the used
postMessage
method, its provided API and documented restrictions: Shouldn't it be able to handle this circular reference?- If so, it is a bug/issue in Chrome/V8 and should be reported.
- If not, then how to detect/prevent such a specific circular reference? (here finally comes the actual fix into play)
from ui5-inspector.
@gtreskas Seems that I finally reached the state you were already in for a bit (my apologies), realizing circular references in general are indeed a problem for port.postMessage
method, which is a different one than mentioned here.
I also noticed this on additional controls, e.g. sap.ushell.components.container.ApplicationContainer
. There are references to the sap.ui.Core
instance within its control data 😮
Possible solutions
- Ignore generally already processed references (what @gtreskas mentioned in the first post): This does not only prevent circular references but also non-circular references to already processed parts. How relevant are those references?
- Detecting and preventing only circular dependencies: Probably decreases the performance (notably?). Is it sufficient?
- Ignoring controls/messages with a circular dependeny (basically catching
JSON.stringify
): User might miss information.
In all three cases, data gets potentially lost, which seems inevitable. Or do I miss something? Is there another solution? What is most favorable?
from ui5-inspector.
@SeeSharpSoft :Yes thank you very much for taking your time and having a look, and stating the facts. Yes you are right is inevitable to keep everything, we have to loose something here, but in this case we should see if what we loose is ok or not. I think the best would be to understand what is the data that we are loosing and if its important for the inspector...
Here we have a tradeoff which we have also to consider , which you rightfully state: Data Amount vs. Performance, as more data we include in the inspector as slower and dangerous (circular refs) it becomes..
The thing is here for me, circular references is no go, this has to be handled and fixed. About performance maybe we need to adjust the existing architecture of the inspector and require "smaller" portions of data packed into "smaller messages". I think this could correct the performance significantly, since we will only require "specific" portions of data and not everything at once...
But maybe the other colleagues may have a better ideas here, what do you say colleagues?
from ui5-inspector.
@gtreskas Just pushed a rework of the message parsing algorithm, removing circular references (2) but also ignoring complex object types.
I am with you: The performance in those cases is super bad even when ignoring all references, but when applying the check for circular references it is literally unusable.
Manual sanity checks with the references change are looking good. I also checked what happens when ignoring all references (1): Then e.g. the context binding information in the bindings tab gets already lost, so this is actually not really an option.
from ui5-inspector.
@SeeSharpSoft Awesome! Thank you very much for your work, i didnt had time lately, but I will try it out and let you know! I will pull the changes from your PR.
from ui5-inspector.
Hello,
This should have been resolved in latest versions.
Best regards,
Jordan
from ui5-inspector.
Related Issues (20)
- firefox support HOT 2
- "Inspect UI5 control" in Crome context menu doesn't work HOT 2
- Support for Focus Mode in Microsoft Edge HOT 2
- Malfunction of the context menu: "Inspect UI5 control" HOT 3
- Empty Control Inspector HOT 6
- Right clicking on a a control, select UI5 Inspector > Inspect UI5 control doesn't show you the line
- UI5 Inspector no longer working in SAPUI5 v1.38. Error control.getMetadata(...).getProperty(...).getDefaultValue is not a function
- Error from highlighter
- Publishing UI5 inspector to Microsoft Extension Store? HOT 1
- firefox support HOT 3
- control inspector doesn't work in incognito
- After opening the ui5 inspector whole page is frozen. HOT 2
- properties with html in the value are rendered as html, and get changed when you click them HOT 1
- element picker
- Inspector doesn't work at new version 1.6.2 HOT 6
- Empty console for inspector HOT 5
- UI5 Inspector doesnt work on Chrome HOT 2
- BaseConfig (sap/base/config/_Configuration) error HOT 1
- Inspector doesn't work at new version 1.6.3 HOT 9
- Close Browser Not Working! HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ui5-inspector.