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

Refactor scope handling #1517

Merged
merged 4 commits into from
Aug 11, 2017

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 6, 2017

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:

  1. All nodes now store their own scope upon initialisation. That way it is no longer necessary to pass along the scope for functions like Node.bind() or Node.run(). Also, Node.findScope() could be removed.
  2. To achieve this without code repetition and in a reliable manner, 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
  3. Nodes that create a new scope now always have 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 problems
  4. BlockStatement 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 the BlockStatement via a new BlockStatement.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.
  5. To remove some code repetition, all three .*Function.* and the two Class.* nodes now share special ancestors Function and Class which again descend from Node

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.

@pshrmn
Copy link

pshrmn commented Aug 6, 2017

This isn't feedback, I just thought that it would be useful to post a link to view the diff without the whitespace changes: https://github.com/rollup/rollup/pull/1517/files?w=1

@lukastaegert
Copy link
Member Author

@pshrmn Thanks, that certainly looks better. I tried to get my IDE to closely match the prevalent style but in some places this was not possible (and I like auto-format). Maybe a few more ESLint rules might help here ;)

@Rich-Harris Rich-Harris merged commit 2395452 into rollup:master Aug 11, 2017
@Rich-Harris
Copy link
Contributor

Thank you! 🎉

@lukastaegert lukastaegert deleted the abolish-scope-parameters branch August 12, 2017 12:50
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.

None yet

3 participants