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

Maintain this context when sequence is callable #1724

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

nathancahill
Copy link
Contributor

@nathancahill nathancahill commented Nov 10, 2017

Fixes #1722

@nathancahill nathancahill changed the title Maintain this context when sequence is callable - fixes #1722 Maintain this context when sequence is callable Nov 10, 2017
@lukastaegert
Copy link
Member

Hi @nathancahill, this looks really nice, thank you a lot! I have thought about it and I agree, the only situation where this can hurt us is where the sequence expression is a direct callee of a call expression, just how you implemented it.

On a side note, this also implies that when we simplify conditional and logical expressions (at least the first case can actually happen, though not as prominently), we need to keep the this context of expressions in mind as well. This is certainly beyond the scope of this fix and might warrant a separate issue. At that point, we should also revisit how those expressions interact with each other.

One thing I was wondering about if it would make sense to add a function test besides the form tests you added that actually checks that the member expression uses the right context. For instance, function tests can mock an imported function that receives the current this context and throws an error if it is not the global object. However, this would only serve some documentary purpose to explain this uncommon usage of the comma operator to future rollup contributors. What do you think?

@lukastaegert lukastaegert added this to the 0.51.x milestone Nov 16, 2017
@nathancahill
Copy link
Contributor Author

Sounds good, I'll add a test for that.

@nathancahill
Copy link
Contributor Author

How does that look?

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Awesome! Even simpler than what I had in mind 😜

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

2 participants