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

fixes https://github.com/riot/riot/issues/2726 #118

Merged
merged 1 commit into from
Jul 9, 2019

Conversation

hudelgado
Copy link
Contributor

It fixes the visitMemberExpression by only prefixing the identifier for the expression if it doesn't exist in scope.
It does that by traversing a MemberExpression looking for the left most Identifier and looking up for it's name in the given scope.

I'm not sure if this was exactly what you had in mind, but i'm okay with improving it accordingly to your suggestions.

Cheers

 - visitMemberExpression only replaces path scope if the identifier for the expression doesn't exist in scope
Copy link
Member

@GianlucaGuarini GianlucaGuarini left a comment

Choose a reason for hiding this comment

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

Thank you. This patch looks great, I have added just a few comments regarding some small improvements we might consider.

* @param {Scope} scope - scope where to search for the identifier
* @returns {boolean} true if the node identifier is defined in the given scope
*/
function nodeIdentifierExistsInScope(node, scope) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this function could be part of the isGlobal function. Could you please try to use it there?

* @returns {boolean} true if the node identifier is defined in the given scope
*/
function nodeIdentifierExistsInScope(node, scope) {
let found = false // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to use this function in a more functional way avoiding the let statement? Probably we can use the return value of types.visit. Let's use mutable variables only if they are only the last resort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have tried it, both with the return type and by trying to catch the exception, but the return type is the starting node used in the traversal, and the exception isn't propagated.

But I will see what I can do with both of your suggestions, I will only be able to work on it after next Wednesday.

But I give you some feedback then.

Cheers

@GianlucaGuarini GianlucaGuarini merged commit 3119039 into riot:bug/riot2726 Jul 9, 2019
@GianlucaGuarini
Copy link
Member

@hudelgado thank you I will have update your patch refactoring a bit your solution. Your help has been really appreciated

@hudelgado
Copy link
Contributor Author

@GianlucaGuarini, i'm glad my help was valuable.
If it's possible i would like to see what's the ending solution, ping me back when you changed so i can check it.
Cheers

@GianlucaGuarini
Copy link
Member

@hudelgado 8c6e99c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants