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

chore(tslint): update tslint to 4.x #13603

Merged
merged 2 commits into from
Dec 27, 2016
Merged

chore(tslint): update tslint to 4.x #13603

merged 2 commits into from
Dec 27, 2016

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Dec 21, 2016

@chuck I did clone tslint, fix the issue and send a PR and then I realize I started from the wrong repo and that it was already fixed in the parent repo.

Anyway this PR should remove the bug.

@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer type: chore labels Dec 21, 2016
@vicb vicb requested a review from chuckjaz December 21, 2016 02:18
@chuckjaz
Copy link
Contributor

This doesn't appear to work on CircleCI

@vicb vicb force-pushed the 1220-tslint branch 5 times, most recently from 75cf30e to 2dbc1b8 Compare December 22, 2016 19:46
@vicb
Copy link
Contributor Author

vicb commented Dec 22, 2016

@chuckjaz PTAL

@@ -11,7 +11,7 @@ import {NgZone} from '@angular/core';
import {global} from './facade/lang';
import {getDOM} from './private_import_platform-browser';

export let browserDetection: BrowserDetection = new BrowserDetection(null);
let browserDetection: BrowserDetection = new BrowserDetection(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the file because classes cannot be used before they are declared (probably worked only as a side-effect of how the export was generated). To fix, remove the initializer as it is redundant with the the call to setup() on line 75.

@vicb vicb force-pushed the 1220-tslint branch 2 times, most recently from cc1bcf1 to c676bfe Compare December 22, 2016 23:49
@vicb
Copy link
Contributor Author

vicb commented Dec 23, 2016

@chuckjaz PTYAL

@@ -135,6 +135,10 @@ export function stringifyElement(el: any /** TODO #9100 */): string {
return result;
}

browserDetection = new BrowserDetection(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Line 75 contains BrowserDetection.setup(); for which setup() is browserDetection = new BrowserDetection(null); Why do we create two?

@chuckjaz chuckjaz added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 23, 2016
@vicb vicb added the action: merge The PR is ready for merge by the caretaker label Dec 23, 2016
@hansl hansl merged commit eed8344 into angular:master Dec 27, 2016
IgorMinar pushed a commit to IgorMinar/angular that referenced this pull request Jan 6, 2017
@vicb vicb deleted the 1220-tslint branch June 9, 2017 17:17
@angular-automatic-lock-bot
Copy link

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-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants