Skip to content

feat: add support for noImplicitThis - #13404

Merged
josephperrott merged 1 commit into
angular:masterfrom
crisbeto:no-implicit-this
Oct 17, 2018
Merged

feat: add support for noImplicitThis#13404
josephperrott merged 1 commit into
angular:masterfrom
crisbeto:no-implicit-this

Conversation

@crisbeto

@crisbetocrisbeto commented Oct 3, 2018

Copy link
Copy Markdown
Member

Adds support for the noImplicitThis compiler option and fixes the compilation errors.

@crisbetocrisbeto added the target: major This PR is targeted for the next major release label Oct 3, 2018
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 3, 2018
devversion
devversion approved these changes Oct 3, 2018
View reviewed changes

@devversiondevversion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. What strict checks are missing? At some we could also just use strict which enables all individual strict flags.

Comment threadsrc/cdk/a11y/interactivity-checker/interactivity-checker.spec.ts
}

function runIf(condition: boolean, runFn: Function): () => void {
function runIf(this: any, condition: boolean, runFn: Function): () => void {

@devversiondevversionOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For a second, I thought that this is an actual parameter.

@crisbeto

crisbeto commented Oct 3, 2018

Copy link
Copy Markdown
MemberAuthor

This should be the last one missing from the strict flag. I talked to @jelbourn about it, but we decided not to use strict, because things can start breaking if the TS team adds another check to it.

@devversion

devversion commented Oct 3, 2018

Copy link
Copy Markdown
Member

@crisbeto Yeah that makes sense. I assumed they would only add these in major releases (where we can expect such breaking changes)

@devversiondevversion added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 3, 2018
jelbourn
jelbourn approved these changes Oct 3, 2018
View reviewed changes

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ngbot

ngbotBot commented Oct 5, 2018

Copy link
Copy Markdown

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto
crisbetoforce-pushed the no-implicit-this branch 2 times, most recently from aa78b13 to 8fd0b55CompareOctober 12, 2018 13:36
@josephperrott

josephperrott commented Oct 16, 2018

Copy link
Copy Markdown
Member

@crisbeto Looks like the e2e tests are failing on this now?

I was able to reproduce the failure locally, though at a glance am unsure of the cause.

@crisbeto
crisbetoforce-pushed the no-implicit-this branch 2 times, most recently from 5e00702 to b3e6a2eCompareOctober 17, 2018 05:56
@crisbeto

crisbeto commented Oct 17, 2018

Copy link
Copy Markdown
MemberAuthor

Fixed @josephperrott.

@crisbeto
feat: add support for noImplicitThis
5154d22
Adds support for the `noImplicitThis` compiler option and fixes the compilation errors.
@josephperrott
josephperrott merged commit 97703be into angular:masterOct 17, 2018
@angular-automatic-lock-bot

angular-automatic-lock-botBot commented Sep 9, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@devversiondevversiondevversion approved these changes
@amcdnlamcdnlAwaiting requested review from amcdnl
@mmalerbammalerbaAwaiting requested review from mmalerba
+1 more reviewer
@jelbournjelbournjelbourn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jelbournjelbourn

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: majorThis PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@crisbeto@devversion@josephperrott@jelbourn@googlebot