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(a19): respecting default standalone flag since angular 19 #10583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

re-alam
Copy link

@re-alam re-alam commented Nov 26, 2024

Added standalone false as default for mock-render and updated isStandalone method to handle angular 19+ default standalone behavior for components.

@re-alam re-alam requested a review from satanTime as a code owner November 26, 2024 16:45
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cad1d0e) to head (a310e94).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master    #10583   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          227       227           
  Lines         4946      4950    +4     
  Branches      1148      1149    +1     
=========================================
+ Hits          4946      4950    +4     

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

@jensweigele
Copy link

I think you need to adapt the unit-tests to get green-light from CI (and approval from @satanTime )

@re-alam re-alam requested a review from jensweigele November 28, 2024 11:52
@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch from 6882b15 to 40ee673 Compare November 28, 2024 14:25
@satanTime
Copy link
Member

satanTime commented Nov 29, 2024

Hi @re-alam, thanks for the fix.

Could stash all commits into one?
Also, please change the message of the commit to:

fix(a19): respecting default standalone flag since angular 19

Thank you in advance.

@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch 2 times, most recently from 2192a71 to 52ef264 Compare November 29, 2024 09:29
@satanTime satanTime enabled auto-merge November 29, 2024 10:15
@satanTime satanTime removed the request for review from jensweigele November 29, 2024 10:17
auto-merge was automatically disabled November 29, 2024 11:18

Head branch was pushed to by a user without write access

@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch from 52ef264 to aa2b371 Compare November 29, 2024 11:18
@satanTime
Copy link
Member

Hi @re-alam, could you sign your commit?

the easiest way is SSH signing: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@satanTime satanTime enabled auto-merge November 29, 2024 11:19
auto-merge was automatically disabled November 29, 2024 11:28

Head branch was pushed to by a user without write access

@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch from aa2b371 to 73d1c95 Compare November 29, 2024 11:28
@re-alam
Copy link
Author

re-alam commented Nov 29, 2024

@satanTime Thank you for approving the change. My commit is now signed. Please feel free to merge when you're able.

@satanTime satanTime changed the title fix(A19): Forgot to flush TestBed fix(a19): respecting default standalone flag since angular 19 Nov 29, 2024
@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch 4 times, most recently from ffe193a to aa6ad24 Compare December 2, 2024 13:14
Added standalone false as default for mock-render and updated isStandalone method to handle angular 19+ default standalone behavior for components.
@re-alam re-alam force-pushed the fix/issues-10306-a19-mock-render-issue branch from aa6ad24 to a310e94 Compare December 2, 2024 13:37
@@ -4,6 +4,7 @@ import './lib/common/ng-mocks-stack';
import './lib/common/ng-mocks-global-overrides';

export * from './lib/common/core.tokens';
export * from './lib/common/func.is-standalone';
Copy link
Member

Choose a reason for hiding this comment

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

It's an internal function and it shouldn't be a part of public interface.

I think the test should be done in a different way. There are some examples in tests folder which verify different behaviors of different angular versions.

Copy link
Author

Choose a reason for hiding this comment

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

@satanTime Thank you for your feedback. I've looked into the tests folder but couldn't find any tests that specifically cover the feature or logic related to Angular version checks. I only found some tests that skip execution based on the Angular version, like this:

if (Number.parseInt(VERSION.major, 10) < 14) {
  it('needs >=a14', () => {
    expect(true).toBeTruthy();
  });
  return;
}

However, I need to ensure code coverage for the Angular 19 version check within the isStandalone function.

I attempted to mock the VERSION object using spyOnProperty, but this wasn't possible as the VERSION object from @angular/core isn't configurable.

Given that isStandalone is an internal function, what do you think about moving the test file to the common folder? Please let me know if there are any other approaches I should consider.

@florestankorp
Copy link

Shouldn't there be an addition to the 'e2e' folder?

image

@endlacer
Copy link

endlacer commented Dec 12, 2024

this should be merged ASAP.
having a testing library block the upgrade of the app to angular 19 is quite the bummer

@GipHub123
Copy link

@endlacer Some people don’t seem to understand that most libraries are maintained on a pro bono basis, without any financial compensation 😠 These developers generously dedicate their free time to create and maintain fantastic development tools that benefit us all.

this has to be merged ASAP. having a testing library block the upgrade of the app to angular 19 is highly unacceptable!

@florestankorp
Copy link

florestankorp commented Dec 12, 2024

this has to be merged ASAP. having a testing library block the upgrade of the app to angular 19 is highly unacceptable!

@endlacer I understand your frustration, but this doesn't have to be blocking: take this fix, build it and temporarily include it in your bundle to import from there. That's what I'm doing for now and it works. DM me if you have questions or need any help with this.

@TomLBarden
Copy link

TomLBarden commented Dec 12, 2024

@endlacer I understand your frustration, but this doesn't have to be blocking: take this fix, build it and temporarily include it in your bundle to import from there. That's what I'm doing for now and it works. DM me if you have questions or need any help with this.

A bit of guidance on this would be hugely appreciated!

@endlacer
Copy link

endlacer commented Dec 12, 2024

this has to be merged ASAP. having a testing library block the upgrade of the app to angular 19 is highly unacceptable!

@endlacer I understand your frustration, but this doesn't have to be blocking: take this fix, build it and temporarily include it in your bundle to import from there. That's what I'm doing for now and it works. DM me if you have questions or need any help with this.

sorry for being too harsh, but we got 4 applications and 2 libraries that all are now blocked bc of something that
a. seems minor
b. already a PR exists to fix it

did not mean any offense!

@florestankorp
Copy link

florestankorp commented Dec 12, 2024

@endlacer I understand your frustration, but this doesn't have to be blocking: take this fix, build it and temporarily include it in your bundle to import from there. That's what I'm doing for now and it works. DM me if you have questions or need any help with this.

A bit of guidance on this would be hugely appreciated!

@TomLBarden here you go:

  1. Clone 'ng-mocks'
  2. Install the dependencies with npm install
  3. Apply the changes suggested in this PR
  4. Build the lib by running: npm run build:all
  5. Copy the folder ng-mocks from dist into your project
  6. Update the imports in your project to point at the newly imported version of 'ng-mocks'
  7. Optional: Disable ESLint for ng-mocks by ignoring the folder:
// eslint.config.js
export default [
    {
        ignores: ["**/ng-mocks/**"]
    }
];

Your tests should be running now with Angular v19 😎

@TomLBarden
Copy link

@florestankorp Thank you!

I wanted to let you know that I did a build based on this PR, and it does not seem to be enough to solve all the issue caused by components being standalone by default in Angular 19.

It works for components directly inside the application, but not for non-standalone components from libraries. I get errors like this one:

@ct-serensia I'm seeing this issue too, did you happen to find a solution for it? I'll continue to investigate this myself, too.

@endlacer
Copy link

My workaround for now is to install back Angular 18 to run unit tests, while everything else (serve, build...) uses Angular 19.

this might work local, but not in a CI pipeline

@TomLBarden
Copy link

My workaround for now is to install back Angular 18 to run unit tests, while everything else (serve, build...) uses Angular 19.

@ct-serensia That's a shame, thanks for the reply though. Looking like I might have to delay this upgrade for now. Unfortunately for us we can't accept the risk of testing against a different version of Angular than we'd be shipping with.

@endlacer
Copy link

ct-serensia i dont even this this would work. the angular 19 migration removes standalone: true flags from the component. when you then run tests with angular 18, the tests would think that this component is standalone-false, which it is not

@szigetib95
Copy link

I don't see any update on this for last 2 weeks as it's being worked on, it would be very helpful to get this problem resolved, as it is stopping the complete Nx migration for all our repos.
Thanks!

@florianjung79
Copy link

For the time being, we continue to declare the standalone components with standalone: true. The tests then run as before and as soon as ng-mocks offers a corresponding fix, the global removal of the declaration is a matter of search and replace.

@szigetib95
Copy link

For the time being, we continue to declare the standalone components with standalone: true. The tests then run as before and as soon as ng-mocks offers a corresponding fix, the global removal of the declaration is a matter of search and replace.

'The global removal of the declaration is a matter of search and replace' - In case your components are standalone, but I need the flag, since its standalone: false.

@florestankorp
Copy link

standalone:

@florianjung79 Do you mean you set your MockRender / MockBuilder to standalone: true? Or your actual components? Can you provide a code snippet?

@florianjung79
Copy link

@florianjung79 Do you mean you set your MockRender / MockBuilder to standalone: true? Or your actual components? Can you provide a code snippet?

The actual components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants