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

fix: unable to bind ngIf in angular 17+ #9314

Merged
merged 4 commits into from
Aug 25, 2024

Conversation

sabrina-10kc
Copy link
Contributor

When component templates contain child templates, they are replaced with a mock template that contains a *ngIf this causes a failure when the component doesn't have NgIf imported.

Though this problem likely exists in some pre-angular 17 situations, it has been causing new failures to appear as people upgrade since 3rd party angular components are starting to use the new control flow syntax and no longer import CommonModule / NgIf.

Instead the mock template now uses @if syntax if angular is 17 or higher.

Fixes #8884

Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9ef4a65) to head (0ba55d0).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #9314   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          227       227           
  Lines         4935      4946   +11     
  Branches      1147      1148    +1     
=========================================
+ Hits          4935      4946   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sabrina-10kc
Copy link
Contributor Author

@satanTime Is there anything I need to change in this PR to help it get in & released ? (other than rebasing of course)

This issue is currently preventing use of errorOnUnknownElements for me

@satanTime
Copy link
Member

Hi @sabrina-10kc,

Thank you for your contribution.
Next week I'll be working on the rep and take a look at the PR.

@donroyco
Copy link

donroyco commented Jul 9, 2024

Any update on this PR, @satanTime ? It would be really helpful to have this fixed.

@mikeysteele
Copy link

It would be great to get this merged. This can cause otherwise good tests to fail depending on when the error is thrown.

@clara-belair
Copy link

Hi @sabrina-10kc,

Thank you for this PR. I've tested it locally and it fix the problem in my projet.

@satanTime Can you tell us if you have the time to merge this PR and release it, please ?

@JsantosDK
Copy link
Contributor

Hello @satanTime when can you approve this?
It's almost 2 months now since your last comment on it.

@satanTime
Copy link
Member

I wish it was so easy.
The thing there is that a line of code which was covered by tests now is simply ignored.
Because of that I need to investigate whether this is the only way to fix the issue, because the line is covered well currently in a17 and a18.

@JsantosDK
Copy link
Contributor

@satanTime after reading this comment, please re-consider moving the version to the approach of angular and the current approach that the library has.

It is a problem for people who want to use it to have to wait for months for something to be fix, because you need to be sure it works on all versions.
It is a problem for people who want to contribute, since they need to ensure that it works on all versions, make the fix so easy that you can understand and then wait for god knows how long until before it is merged.

The angular team just gave a speech a few days ago saying that they realize by time.
Everybody that uses things in the angular ecosystem is used to this way of release management.

Me and others want to contribute, but the current approach means everything will always fall on you.
You are blocking other from helping because you need to be 100% sure of everything, at the same time you don't have time.

Yesterday, I installed the lib on a new machine to contribute with the specs and it took over 1h. The approach to develop the lib already takes 6GB in docker to be run, this is not a normal amount of space for a library that like you say, is just a wrapper for making life easier. Will this keep going until the machine reaches it's limit?

People want to help, but it seems you don't want to be help, you don't really want others to collaborate with you.

What would it take to help?
Not just on this issue, but on the lib itself?
Or do you really want to keep going like, until eventually the lib faces so many issues that it dies out due to your lack of time?

I'm working on the schematics, but if there is anything I can help, tell.
And please, consider the change in versioning since the consumers of the lib don't agree with you and it doesn't match the way the angular ecosystem moves.

@satanTime
Copy link
Member

satanTime commented Aug 23, 2024

Not sure I understand your point. Old syntax is support in all angular versions it wasn't removed from a17 or a18. So I'm not sure how release change can improve that. Still both syntax should be supported for a18 and a17.

@JsantosDK
Copy link
Contributor

My main issue is that this PR has been opened for a long time without any feedback from anybody.

Really seems like you don't have the time to support the lib alone, but don't allow much help.

@satanTime
Copy link
Member

Feel free to help, a test which would cover both beaches instead of ignoring one would help.

@JsantosDK
Copy link
Contributor

Ok, will try to get it done between today and tomorrow.

@satanTime
Copy link
Member

Great. Thanks. Is that branch cannot be covered at all - that’s also fine. I just need to be sure that that’s exactly like that.

@JsantosDK
Copy link
Contributor

JsantosDK commented Aug 24, 2024

@satanTime I reviewed this with the focus on reaching the other part of the branch.
Only way to do so, was modify the specs so that it has 2 branches, one for angular 16 and below, and another for angular 17 and above.

When I did this, I got the following results:

Angular 17+ branch:

  • Original specs pass;
  • Added spec passes (expected, due to the if statement exiting before the test is reached);

Angular 16 and below:

  • Original specs pass(expected, due to the if statement exiting before the test is reached);
  • Added spec fails with the same error as originally: NG0303: Can't bind to 'ngIf' since it isn't a known property of 'ng-template' (used in the 'TargetComponent' component template);

From what I can tell, I'm worried that if this PR is merged, and a new version is released, in an Angular 16 or below app, it will break it. I may try to make a build and tested this myself with an actual use case, but would like your input on this before proceeding.

Note: other that using this testing logic, I don't think there is a way to reach the other branch statement.

In example you can find the spec file I used:

import {
  Component,
  ContentChild,
  NgModule,
  TemplateRef,
  VERSION,
} from '@angular/core';

import { MockBuilder, MockRender, ngMocks } from 'ng-mocks';

// @see https://github.com/help-me-mom/ng-mocks/issues/8884
describe('issue-8884', () => {
  ngMocks.throwOnConsole();

  describe('with a16 and below (added by me)', () => {
    if (Number.parseInt(VERSION.major, 10) > 16) {
      it('needs a16 or below', () => {
        expect(true).toBeTruthy();
      });

      return;
    }

    @Component({
      selector: 'app-target',
      template: `<ng-template [ngTemplateOutlet]="content" />`,
    })
    class TargetComponent {
      @ContentChild('content', {} as never)
      content?: TemplateRef<any>;
    }

    @NgModule({
      declarations: [TargetComponent],
    })
    class TargetModule {}

    beforeEach(() => MockBuilder(null, TargetModule));

    it('should create', () => {
      MockRender(`<app-target>Test content</app-target>`);

      expect(ngMocks.findInstance(TargetComponent)).toBeTruthy();
    });
  });

  describe('with a17+', () => {
    if (Number.parseInt(VERSION.major, 10) < 17) {
      it('needs a17+', () => {
        expect(true).toBeTruthy();
      });

      return;
    }

    describe('when standalone component does not import NgIf', () => {
      @Component({
        selector: 'app-standalone',
        standalone: true,
        template: `<ng-template [ngTemplateOutlet]="content" />`,
      })
      class StandaloneComponent {
        @ContentChild('content', {} as never)
        content?: TemplateRef<any>;
      }

      beforeEach(() => MockBuilder(null, StandaloneComponent));

      it('should create', () => {
        MockRender(`<app-standalone>Test content</app-standalone>`);

        expect(ngMocks.findInstance(StandaloneComponent)).toBeTruthy();
      });
    });

    describe('when NgIf is not available to a component in a module', () => {
      @Component({
        selector: 'app-target',
        template: `<ng-template [ngTemplateOutlet]="content" />`,
      })
      class TargetComponent {
        @ContentChild('content', {} as never)
        content?: TemplateRef<any>;
      }

      @NgModule({
        declarations: [TargetComponent],
      })
      class TargetModule {}

      beforeEach(() => MockBuilder(null, TargetModule));

      it('should create', () => {
        MockRender(`<app-target>Test content</app-target>`);

        expect(ngMocks.findInstance(TargetComponent)).toBeTruthy();
      });
    });
  });
});

@satanTime satanTime enabled auto-merge August 25, 2024 11:44
@satanTime satanTime merged commit fd98e37 into help-me-mom:master Aug 25, 2024
6 checks passed
@satanTime
Copy link
Member

v14.13.1 has been released and contains a fix for the issue. Feel free to reopen the issue or to submit a new one if you meet any problems.

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.

Bug: MockRender throws an error on mat-tab
6 participants