Use reflectComponentType as a Fallback to Detect Signals#11909
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11909 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 227 228 +1
Lines 4967 4988 +21
Branches 1154 1158 +4
=========================================
+ Hits 4967 4988 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Watercycle I've tried your version in my ngMocks bugs repro project (https://github.com/iamkinetic/NgMocksBugs) and all of the tests that were failing are now all passing! I'll have to try it with a real project, but this seems very promising! |
|
@Watercycle I've tested your solution some more:
I've updated my repro project to demonstrate the issues with query signals. They cause all of the tests to fail. Edit: using any of the query signal is causing some issues even when they are private in a component. |
|
@satanTime So is something still missing (beside time I guess lol)? Do you think you'll be able to review and merge this soon? |
|
@satanTime can you please take another look at this PR to see if there are any additional concerns needing to be addressed? |
|
Hi @sgbrown, only to make it green, I see that coverage has dropped. unfortunately, the page isn't loaded atm: https://coveralls.io/builds/74925913/source?filename=libs%2Fng-mocks%2Fsrc%2Flib%2Fresolve%2Fcollect-declarations.ts but that's the only thing to fix. |
Ah, yeah, I kind of put tests off as part of the uncertainty with the solution, in how coverage is reported across all the tests, and because truly testing this requires building a throwaway lib. I'll try at least scoping a test to the newer Angular versions and/or mocking out |
@Watercycle Have you had a chance to fix your coverage? |
|
@satanTime It's green, now! Let me know if anything else needs to be done! And sorry for the horrendous delay - got addicted to motorcycles. Fortunately for this branch it started snowing 🤣 |
|
@satanTime any chance of getting this PR merged and a new release published soon since it is now green? |
|
Any chance for a new release with the fix ? Our internal libraries are migrating to A20 build and all imported components with container queries are failing. Using .replace(, ) to manually mock each component without the queries seems to work, but it's quite time consuming. 🙏 |
|
@satanTime could you please take another look at this PR to see if it needs anything else. The checks are all green and this update would be greatly appreciated. I appreciate your time and effort. I hope you're doing well |
|
Hi @sgbrown, the fix is okay. But commits should be signed. |
1c63b76 to
7cf6cfd
Compare
|
@satanTime Did you mean to push those up as signed commits? I'm happy to if it's giving you issues! |
|
Hi @Watercycle, that's right. Please do. |
|
@Watercycle do you think you'll be able to sign your commits soon so we can get this in? |
2a51cd5 to
c469d38
Compare
(at the cost of some tree-shaking, most likely)
c469d38 to
754e4ff
Compare
|
@satanTime Sorry for the delay! Re-based on master with verified commits. After this PR, would you be open to me making another PR cherry-picking the changes from #8771? 👀 Both will be needed for full signal support. |
|
@satanTime Could you please approve and merge this PR when you get a chance? The commits are now signed which I believe to be the last thing you were waiting for. @Watercycle Thank you for signing the commits |
|
v14.15.0 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. |
|
@satanTime oddly, with @Watercycle's version I had everything working but with v14.15.0 I get |
Yes, this merge request has already been merged and the related issue as well. I also had one of my test giving me this error, but it was the only one out of a thousand. |
|
@Watercycle ah so that's what the deal is, I assumed that the released version contained everything from this PR so was wondering. Well ok that makes sense, basically known issue then. I hope you get a chance to solve this, thanks! |
|
Sorry for the confusion! #12875 will add that next bit of compatibility. @satanTime Let me know if you have any concerns! |
This PR attempts to at least partially address #9698.
I saw this comment and #8818, which leads me to believe this is the wrong solution; but, it does seem to fix the problem on a fresh Angular app (
ng generate app my-app+ng generate library my-lib) and the larger Angular project that I care about - using NG 19, AOT, Webpack, and Karma.For those interested, I built the package and published it here. You can use it by updating your
package.jsonwith the following:and running
npm upgrade ng-mocks. Alternative workarounds are noted here. (Edit: Use0.0.1to include the changes from #8771).This has been tricky to validate between (seemingly) NG cache, NPM cache, and having to restart the test runner each time. That's mostly my inexperience with NPM packaging; but, it has led me down several false paths.