Skip to content

new lint rule: no static this#29692

Merged
samouri merged 2 commits intoampproject:masterfrom
samouri:static-this
Aug 7, 2020
Merged

new lint rule: no static this#29692
samouri merged 2 commits intoampproject:masterfrom
samouri:static-this

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Aug 5, 2020

Closure Compiler doesn't support static this. It doesn't explode either, it just behaves strangely at runtime.
This lint rule should help up catch it early.

Most of the credit goes to Justin for writing the meat of the rule.

cc @kristoferbaxter

Blocked by #29695

@google-cla google-cla bot added the cla: yes label Aug 5, 2020
@samouri samouri self-assigned this Aug 5, 2020
@samouri samouri requested a review from jridgewell August 5, 2020 22:11
@samouri samouri marked this pull request as ready for review August 5, 2020 22:15
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Aug 5, 2020

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/eslint-rules/no-static-this.js

Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Nice work @samouri and @jridgewell!

@kristoferbaxter
Copy link
Copy Markdown
Contributor

The remaining condition flagged was addressed in master, should be able to merge it in and then we can commit this ESLint rule.

@samouri samouri merged commit 0f4af54 into ampproject:master Aug 7, 2020
@samouri samouri deleted the static-this branch August 7, 2020 18:43
@samouri
Copy link
Copy Markdown
Member Author

samouri commented Aug 7, 2020

Note: this completes the AIs from #29499

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* new lint rule: no static this

* remove a harmless instance of static this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants