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

Fix reference error issue when setting a new value for property #546

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@ahmadov
Copy link
Contributor

ahmadov commented Nov 22, 2019

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Nov 22, 2019

FYI @EricCornelson it looks like the $s disappeared when moving this code to its own class.

Copy link
Member

roblourens left a comment

Thanks, I would rather keep it the way it was before using ${}. I assume that if we pass strings to the "arguments" here, they would always be interpreted as strings.

@ahmadov

This comment has been minimized.

Copy link
Contributor Author

ahmadov commented Nov 22, 2019

Yes, first I wanted to just add the missing ${} but then I realized the type of the value argument is already a string. So, If we put the missing $, the function would be like that: this[...] = ${value} and putting $ is not enough because if the value is "foo" the result will be this[...] = foo which is also undefined.

The way to solve that issue is surrounding the ${value} with double-quotes such as this[...] = "${value}" and in that case, it'd be always interpreted as strings.

I followed the other way because Chrome also uses the way "arguments". I think the underlying problem is type of the value is always a string.

@ahmadov ahmadov force-pushed the ahmadov:fix_set_property_value branch from c8457db to 6b4de3c Nov 25, 2019
@ahmadov

This comment has been minimized.

Copy link
Contributor Author

ahmadov commented Nov 25, 2019

Hi @roblourens,
I got your point and changed the code back to ${} as it was before.

Copy link
Member

roblourens left a comment

Thanks!

@roblourens roblourens merged commit 85f93ad into microsoft:master Nov 25, 2019
2 checks passed
2 checks passed
license/cla All CLA requirements met.
vscode-chrome-debug-core-CI #20191125.1 succeeded
Details
@roblourens roblourens added this to the November 2019 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.