angularathens / 2nd-workshop-angular-contrib

2nd workshop on how to contribute to Angular
4 stars 1 forks source link

refactor(docs-infra): enable tslint rules for the angular.io app #8

Open gkalpak opened 4 years ago

gkalpak commented 4 years ago

Make the angular.io app source code compatible with pending tslint rules and enable/disable those rules (in aio/tslint.json):

Rules to enable:

NOTE: This issue can be split up per rule.

PREREQUISITE: This requires angular/angular#38993 to have been merged. Done.

bampakoa commented 4 years ago

@gkalpak Prerequisite PR merged 🎉

gkalpak commented 4 years ago

Instructions

  1. Open aio/tslint.json.
  2. Update the config for one rule.
  3. Run yarn --cwd=aio ng lint (to run linting checks on the angular.io app).
  4. Fix one or more failures.
  5. Go back to step (3) until you're fed up or there are no more failures (whichever comes first 😛).
  6. Unless all failures for the specific rule are fixed (i.e. yarn --cwd=aio ng lint reports no failures), remember to revert the changes in aio/tslint.json before committing (otherwise, the CI checks will fail).
  7. Commit and submit PR.
tasos-ale commented 4 years ago

@gkalpak @bampakoa I think I can try my luck with this issue.

tasos-ale commented 4 years ago

@jkrgS @profanis Here is the diff from the changes we talked about during the workshop.

diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts
index 03ab16fe9f..aefa6466b0 100644
--- a/aio/src/app/app.component.spec.ts
+++ b/aio/src/app/app.component.spec.ts
@@ -635,61 +635,69 @@ describe('AppComponent', () => {
     });

     describe('aio-toc', () => {
-      let tocContainer: HTMLElement|null;
-      let toc: HTMLElement|null;
-
-      const setHasFloatingToc = (hasFloatingToc: boolean) => {
+      const getHasFloatingTocAndInit = (hasFloatingToc: boolean) => {
         component.hasFloatingToc = hasFloatingToc;
         fixture.detectChanges();

-        tocContainer = fixture.debugElement.nativeElement.querySelector('.toc-container');
-        toc = tocContainer && tocContainer.querySelector('aio-toc');
-      };
+        const tocContainer = fixture.debugElement.nativeElement.querySelector('.toc-container');
+        const toc = tocContainer && tocContainer.querySelector('aio-toc');

+        if (toc === null) {
+          throw new Error('aio-toc element is null');
+        }
+        if (tocContainer === null) {
+          throw new Error('aio-container element is null');
+        }

-      beforeEach(() => {
-        tocContainer = null;
-        toc = null;
-      });
+        return {
+          toc: (toc as HTMLElement),
+          tocContainer: (tocContainer as HTMLElement),
+        };
+      };

       it('should show/hide `<aio-toc>` based on `hasFloatingToc`', () => {
-        expect(tocContainer).toBeFalsy();
-        expect(toc).toBeFalsy();
-
-        setHasFloatingToc(true);
+        let toc;
+        let tocContainer;
+        const _getHasFloatingTocAndInit = (hasFloatingToc: boolean) => {
+          const tocElements = getHasFloatingTocAndInit(hasFloatingToc);
+          toc = tocElements.toc;
+          tocContainer = tocElements.tocContainer;
+        };
+
+        _getHasFloatingTocAndInit(true);
         expect(tocContainer).toBeTruthy();
         expect(toc).toBeTruthy();

-        setHasFloatingToc(false);
+        _getHasFloatingTocAndInit(false);
         expect(tocContainer).toBeFalsy();
         expect(toc).toBeFalsy();
       });

       it('should have a non-embedded `<aio-toc>` element', () => {
-        setHasFloatingToc(true);
-        expect(toc!.classList.contains('embedded')).toBe(false);
+        const { toc } = getHasFloatingTocAndInit(true);
+        expect(toc.classList.contains('embedded')).toBe(false);
       });

       it('should update the TOC container\'s `maxHeight` based on `tocMaxHeight`', () => {
-        setHasFloatingToc(true);
+        const { tocContainer } = getHasFloatingTocAndInit(true);

         component.tocMaxHeight = '100';
         fixture.detectChanges();
-        expect(tocContainer!.style.maxHeight).toBe('100px');
+        expect(tocContainer.style.maxHeight).toBe('100px');

         component.tocMaxHeight = '200';
         fixture.detectChanges();
-        expect(tocContainer!.style.maxHeight).toBe('200px');
+        expect(tocContainer.style.maxHeight).toBe('200px');
       });

       it('should restrain scrolling inside the ToC container', () => {
         const restrainScrolling = spyOn(component, 'restrainScrolling');
         const evt = new WheelEvent('wheel');
+        const { tocContainer } = getHasFloatingTocAndInit(true);

-        setHasFloatingToc(true);
         expect(restrainScrolling).not.toHaveBeenCalled();

-        tocContainer!.dispatchEvent(evt);
+        tocContainer.dispatchEvent(evt);
         expect(restrainScrolling).toHaveBeenCalledWith(evt);
       });

@@ -697,7 +705,7 @@ describe('AppComponent', () => {
         const loader = fixture.debugElement.injector.get(ElementsLoader) as unknown as TestElementsLoader;
         expect(loader.loadCustomElement).not.toHaveBeenCalled();

-        setHasFloatingToc(true);
+        getHasFloatingTocAndInit(true);
         expect(loader.loadCustomElement).toHaveBeenCalledWith('aio-toc');
       });
     });
diff --git a/aio/src/app/custom-elements/announcement-bar/announcement-bar.component.spec.ts b/aio/src/app/custom-elements/announcement-bar/announcement-bar.component.spec.ts
index ddd274d0b2..b6acde6ec8 100644
--- a/aio/src/app/custom-elements/announcement-bar/announcement-bar.component.spec.ts
+++ b/aio/src/app/custom-elements/announcement-bar/announcement-bar.component.spec.ts
@@ -102,11 +102,21 @@ describe('AnnouncementBarComponent', () => {
     });

     it('should display an image', () => {
-      expect(element.querySelector('img')!.src).toContain('dummy/image');
+      const imgElement = element.querySelector('img');
+      if (imgElement !== null) {
+        expect(imgElement.src).toContain('dummy/image');
+      } else {
+        expect(imgElement).toThrow(new Error('Image does not exist'));
+      }
     });

     it('should display a link', () => {
-      expect(element.querySelector('a')!.href).toContain('link/to/website');
+      const anchorElement = element.querySelector('a');
+      if (anchorElement !== null){
+        expect(anchorElement.href).toContain('link/to/website');
+      } else {
+        expect(anchorElement).toThrow(new Error('Anchor element does not exist'));
+      }
     });
   });
 });
diff --git a/aio/tslint.json b/aio/tslint.json
index 044f74da1b..a9347787b9 100644
--- a/aio/tslint.json
+++ b/aio/tslint.json
@@ -64,7 +64,7 @@
       "ignore-params"
     ],
     // TODO(gkalpak): Fix the code and enable this to align with CLI. (Failures: 59)
-    // "no-non-null-assertion": true,
+    "no-non-null-assertion": true,
     "no-redundant-jsdoc": true,
     // TODO(gkalpak): Fix the code and remove this to align with CLI.
     "no-switch-case-fall-through": true,

And they break with this error:

...
Chrome Headless 84.0.4147.0 (Mac OS 10.15.7) AppComponent with proper DocViewer aio-toc should show/hide `<aio-toc>` based on `hasFloatingToc` FAILED
    Error: aio-toc element is null
...

I have never used jasmine, so I am not sure how it works and I can not understand why with these changes toc is null.

gkalpak commented 4 years ago

Thx for working on this, @tasos-ale! Could you create a draft PR, so we can continue the discussion there? 🙏

tasos-ale commented 4 years ago

Sure @gkalpak, I will try to send it tomorrow.

tasos-ale commented 4 years ago

@gkalpak https://github.com/angular/angular/pull/39307

bampakoa commented 3 years ago

@tasos-ale we will be having another workshop next Thursday 😃 I have seen that you are currently working on your PR but you are more than welcome to attend if you would like any insights on the issue. We are here to help 👌 Let me know if you want so that I can send you a Zoom link for the event.

tasos-ale commented 3 years ago

Thanks @bampakoa for the invite, but I think I will not be able to make it. I will send you a message if something changes.