Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request will refactor and clean up several aspects of scope handling as described below. The overall goal is to make future extensions easier, especially with regard to improving the tree-shaking aspects of rollup. This pull-request also includes pull request #1498 that needed to be adjusted for this refactoring.
Key changes:
Node.bind()
orNode.run()
. Also,Node.findScope()
could be removed.Node.initialise(parentScope)
has been refactored to follow a template method pattern. Namely,Node.initialise()
now calls in order the following three functions, all of which may be overridden by concrete nodes:Node.initialiseScope(parentScope)
The default implementation stores parentScope in this.scope. Nodes may decide to create a new scope here instead
Node.initialiseNode(parentScope)
No default implementation, can be overridden to add other initialisation without overriding the other two functions
Node.initialiseChildren(parentScope)
By default, all child nodes are initialised with this.scope. Can be overridden to e.g. provide children with a different scope
this.scope
set to this new scope, not their parent scope. It appears the only cases where the parent scope is needed is when declarations are added to a scope. Since this only happens during.initialise()
where the parent scope is available as a parameter, this decision did not pose any problemsBlockStatement
handling has been changed to follow more of a tell, don't ask style. Namely, block statements no longer need to know about all of their possible parents. Instead, block statement parents have the option to set the scope of theBlockStatement
via a newBlockStatement.initialiseAndReplaceScope(scope)
alternative initialisation method. Presence of this methods is checked for via duck-typing, not by checking the actual node type. Also, parameter initialisation is now handled by the respective parents..*Function.*
and the twoClass.*
nodes now share special ancestorsFunction
andClass
which again descend fromNode
I hope you like the changes and of course I am very interested in feedback and open for suggestions. Even though this pull request does not add any new functionality (besides the fix for switch scopes from #1498), it will help in implementing improvements to the way call expressions and member expressions are currently handled which may in the future allow to correctly handle
this
in sub-scopes of constructors or allow reassigning variables without it being interpreted as a side-effect. At the same time, quite a bit of code may be cleaned up. The way I want to go is what is currently implemented in eslint-plugin-tree-shaking.