-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fixes #772 Instance target bug #778
Conversation
With the new export/import approach, we do not export instances that has no additional info to their bases, and because of this we can lose pointer target information. We introduced a combined id (guidOfTheRootOfInstantiation@relativePath) that can be used during the import to successfully recreate such relations.
@@ -1031,6 +1061,8 @@ define(['common/util/assert', 'blob/BlobConfig'], function (ASSERT, BlobConfig) | |||
core.setPointer(node, keys[i], null); | |||
} else if (nodes[target] && toRemoveGuids.indexOf(target) === -1) { | |||
core.setPointer(node, keys[i], nodes[target]); | |||
} else if (isCombinedId(target)) { | |||
core.setPointer(node, keys[i], getCombinedTarget(target)); | |||
} else { | |||
console.log('error handling needed???!!!???'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about bringing in that logger and/or throwing an exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking the logger, but either the public API should be changed, or some global logger should be created.
On the other hand the given else branch is considered (again) a never run into branch, that is why I left it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about throwing an exception?
It was probably meant not to be entered before either but it did.
Now if someone imports and doesn't pay attention to the logs they might end up with an incomplete model
Looks good besides two minor notes. Coverage looks good too. |
Fixes #772 Instance target bug
Thanks a bunch |
The commits should be cherry-picked into the master as well.