GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — style(aio): enforce strict TypeScript checks by petebacondarwin · Pull Request #21342 · angular/angular

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented Jan 5, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Unfortunately the version of @angular/service-worker that we are using (^1.0.0-beta.16)
contains an implicit any which is the only thing preventing the enforcement of noImplicitAny.
This looks like it is fixed in v5+ of the package.

Closes #20646

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@petebacondarwin petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jan 5, 2018
@petebacondarwin
petebacondarwin requested a review from gkalpak January 5, 2018 23:01
@googlebot googlebot added the cla: yes label Jan 5, 2018
@mary-poppins

mary-poppins commented Jan 5, 2018

Copy link
Copy Markdown

You can preview 78cebb9 at https://pr21342-78cebb9.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak
gkalpak suggested changes Jan 9, 2018
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Overall LGTM 👍
Just a couple of nits and a linting error.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/app.component.spec.ts Outdated

@gkalpak gkalpak Jan 8, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LoL
(Maybe move the first ! to the line above?)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 9, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

if you insist

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Jan 9, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't 😇

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/sw-updates/sw-updates.service.spec.ts Outdated

@gkalpak gkalpak Jan 8, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How come tslint doesn't complain about Function. It usually prefers a specific function signature (e.g. () => void) 😃

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No idea but I changed it to VoidFunction just to keep you happy.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread aio/src/app/layout/doc-viewer/doc-viewer.component.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can avoid the as any (here and below), by giving prop a type of keyof CSSStyleDeclaration (which also improves type-safety for the animateProp() function 🎉).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

not quite so simple since length and parentRule properties are readonly :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Jan 10, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ooops 😞

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Arguably, this points out a bug in the code :-) I have put a check for those two properties.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/shared/custom-icon-registry.spec.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why is this needed? Doesn't getNamedSvgIcon return an observable of SVGElements?
Is it for comparing with the result of createSvg()? Wouldn't it be more appropriate to "fix" createSvg() instead:

 function createSvg(svgSrc) {
   const div = document.createElement('div');
   div.innerHTML = svgSrc;
-  return div.querySelector('svg');
+  return div.querySelector<SVGElement>('svg')!;
 }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

+1
I fixed by relaxing the return type of createSvg(svgSrc: string): SVGElement

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/shared/location.service.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Wouldn't it be preferrable to have go(url?: string|null)? Do we need to explicitly pass undefined?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So we do have explicit tests for each of these scenarios: https://github.com/angular/angular/blob/master/aio/src/app/shared/location.service.spec.ts#L272
But looking at the code, we never call with undefined or null so actually let's make it tighter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/app/shared/location.service.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why do we need the |undefined part?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think we do 😱 - perhaps I was being over-zealous!

Actually we have a test for this very scenario: https://github.com/angular/angular/blob/master/aio/src/app/shared/location.service.spec.ts#L339

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/app/shared/location.service.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

AFAICT, the value type should be string|null|undefined.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

OK, so as far as we use this we never pass null, so instead of relaxing the params type I am going to fix the use of double equals below this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/app/shared/scroll-spy.service.spec.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit:
|undefined is somewhat confusing imo (and unnecessarily less strict - which is not a concern is tests of course). If it is just for avoiding the "used before being assigned" error, it might be more clear to initialize the value (e.g. let lastActiveItem: ScrollItem|null = null) or use ! when using it (e.g. expect(lastActiveItem!))?

(I understand this stems from TypeScript's current limitation to infer that a value might actual be set (through a synchronous function call) and falls under the "Super Nit" category, so feel free to ignore 😁)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

OK, so initially I was concerned that we actually check that this value is null at the end of the test, so if none of the subscription handlers were called then this pass despite not being correct. But I now notice that before we check for it becoming null we check that it is some non-null value, so if none of the handler ran then the first expectation would fail before we get the false positive.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/shared/scroll.service.spec.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Missing space before string. (While you are at it, you can remove the double-space before => 😁)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

+1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/search/interfaces.ts Outdated

@gkalpak gkalpak Jan 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

OOC, under what circumstances can a search result have no associated title?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@petebacondarwin petebacondarwin Jan 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Never :-) Or at least only if the data is bad.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@IgorMinar

IgorMinar commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

If I understand Pete's comment correctly, this is blocked on #19795. right?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin

petebacondarwin commented Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

@IgorMinar : No, this is not blocked on #19795 but we just can't turn on the final noImplicitAny rule until that lands. Our code is all now compliant with that rule, we just don't have it turned on.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin
petebacondarwin force-pushed the aio-strict branch from d1aa41e to 40a4e68 Compare January 10, 2018 10:45
@mary-poppins

mary-poppins commented Jan 10, 2018

Copy link
Copy Markdown

You can preview d1aa41e at https://pr21342-d1aa41e.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Jan 10, 2018

Copy link
Copy Markdown

You can preview 40a4e68 at https://pr21342-40a4e68.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin
petebacondarwin force-pushed the aio-strict branch from 40a4e68 to 124c34e Compare January 10, 2018 14:55
@mary-poppins

mary-poppins commented Jan 10, 2018

Copy link
Copy Markdown

You can preview 124c34e at https://pr21342-124c34e.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@IgorMinar

IgorMinar commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

@petebacondarwin I see. that makes sense. thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak
gkalpak approved these changes Jan 11, 2018
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM (as soon as CI is green 😃)
🎉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gkalpak gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 11, 2018
@petebacondarwin
petebacondarwin force-pushed the aio-strict branch from 124c34e to d133c0e Compare January 11, 2018 14:29
@mary-poppins

mary-poppins commented Jan 11, 2018

Copy link
Copy Markdown

You can preview d133c0e at https://pr21342-d133c0e.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin

petebacondarwin commented Jan 11, 2018

Copy link
Copy Markdown
Contributor Author

oh I forgot about the e2e scripts
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin
style(aio): enforce strict TypeScript checks
537e4d0 
Closes angular#20646
@petebacondarwin
petebacondarwin force-pushed the aio-strict branch from d133c0e to 537e4d0 Compare January 11, 2018 18:46
@mary-poppins

mary-poppins commented Jan 11, 2018

Copy link
Copy Markdown

You can preview 537e4d0 at https://pr21342-537e4d0.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gkalpak

gkalpak commented Jan 11, 2018

Copy link
Copy Markdown
Member

File sizes were affected for some reason 😁
The limits need to be updated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 12, 2018
alexeagle pushed a commit that referenced this pull request Jan 12, 2018
@petebacondarwin @alexeagle
style(aio): enforce strict TypeScript checks (#21342)
71dd92b 
Closes #20646

PR Close #21342
@alexeagle alexeagle closed this in c5c6d84 Jan 12, 2018
@petebacondarwin
petebacondarwin deleted the aio-strict branch January 16, 2018 11:04
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable strictNullChecks mode for aio codebase

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@petebacondarwin @mary-poppins @IgorMinar @gkalpak @googlebot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.