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.

Sign up for GitHub

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

fix(utils): handle 0 as a reference value #325

Merged
merged 1 commit into from
Oct 2, 2019
+18 −8
Merged

fix(utils): handle 0 as a reference value #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions __tests__/utils/resolveObject.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,5 +294,15 @@ describe('utils', () => {
]));
});

it('should handle 0', () => {
GroupMessages.clear(PROPERTY_REFERENCE_WARNINGS);
var test = resolveObject({
"test": { "value": "{zero.value}" },
"zero": { "value": 0}
});
expect(GroupMessages.fetchMessages(PROPERTY_REFERENCE_WARNINGS).length).toBe(0);
expect(test.test.value).toBe(0);
});

});
});
16 changes: 8 additions & 8 deletions lib/utils/resolveObject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ function compile_value(value, stack) {
// Find what the value is referencing
ref = selfRef(variable.trim(), updated_object);

if (ref) {
if (typeof ref !== 'undefined') {
if (typeof ref === 'string') {
to_ret = value.replace(match, ref);

Expand Down Expand Up @@ -150,24 +150,24 @@ function selfRef(str, obj) {
context = current_context.join(options.separator);

for (i = 0; i < array.length; i++) {
if (ref[array[i]]) {
// Check for undefined as 0 is a valid, truthy value
if (typeof ref[array[i]] !== 'undefined') {
ref = ref[array[i]];
} else {
ref = obj;
// set the reference as undefined if we don't find anything
ref = undefined;
break;
}
}

// If the reference doesn't change then it means
// we didn't find it in the object
if (ref === obj) {
if (typeof ref === 'undefined') {
GroupMessages.add(
PROPERTY_REFERENCE_WARNINGS,
"Reference doesn't exist: " + context + " tries to reference " + str + ", which is not defined"
);
} else {
return ref;
}

return ref;
Copy link

Choose a reason for hiding this comment

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

It looks like in the previous version a bad ref would result in this function returning "obj" (ref = obj on line 156, return ref here). Now it doesn't return anything if there's a bad ref. Don't know if this is good or bad, just wondering if it might have the potential to break something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point I forgot to mention that part. So I don't know why we were returning the whole object, if you look at line 91 we were testing for truthiness, which if we passed an object back would always be true. Then in line 131 we set to_ret = ref which would be that object as well and return it. This is inside a string.replace function, and returning an object does nothing to the string. So this was always "working", but not how we thought it was...

Copy link
Member Author

Choose a reason for hiding this comment

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

And hopefully all our glorious unit tests would catch anything major, but seems everything is in order.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is so hard to understand - I'm concerned about the out-of-scope variable (updated_object) used within. I'm pretty sure I added the code that sets ref = obj as part of the work to provide better error output (although blame does not seem to think so), but I'm not clear how it interfaces with the updated_object variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so updated_object is created immediate upon calling resolveObject. It is a deep clone of the object being resolved, to prevent any changes that are made during this process from trickling back outside the resolveObject scope. I think everything is ok as is...

}


Expand Down