-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: add Angular Signals CT Harness for Angular 17.2 and up for users to be able to use Angular Signals within their component tests #29621
Conversation
Passing run #55968 ↗︎
Details:
Review all test suite changes for PR #29621 ↗︎ |
ace8fa2
to
30d95ad
Compare
1a23468
to
00c2884
Compare
add changelog entry and build binary [run ci] rename angular18 to angular-signals until we are able to merge back into core package [run ci] fix linting job [run ci] make sure angular-signals harness is copied to cli after build [run ci] add project fixture directory to angular 18 and build binaries for newly named branch run ci update cache [run ci] bust nx cache [run ci] bust cache on linux [run ci] try busting the cache... again [run ci] usually helps when you have the correct build output... [run ci] fix issue where component internal props were getting blown away when user would not set prop in componentProperties [run ci]
d0ebc25
to
c6cba8b
Compare
…signals-ct-harness
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.
We need to confirm a few of the items I left comments on before merging
<p data-cy="firstName"> {{ acquaintance.firstName }} </p> | ||
<p data-cy="lastName"> {{ acquaintance.lastName }} </p> | ||
<p data-cy="age"> {{ acquaintance.age }} </p> | ||
</li> |
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.
Angular also introduced something called Control Flow that we should use moving forward. Angular's compiler handles this for us BUT it would be always ideal to validate its working correctly in our newest Angular CT libs.
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 can add a system test to validate
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.
added in 3f93d59
effect(() => { | ||
// there is a bug in Angular 17 that doesn't rerender the signal when set outside the component context | ||
// this is resolved in Angular 18. adding an effect() causes the template to be update when the signal is updated | ||
console.log(`The user is: ${JSON.stringify(this.user())}`) |
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.
Are you wanting to keep these console.logs
in here?
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.
in this case yes, since it references the signal it forces a recalculation of the value and therefor a repaint. We don't need it in angular 18, but in 17 there is a bug where the signal being updated does not cause a repaint
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'm assuming it is possible to spy on the output()
without using createOutputSpy
? We should add a test around this scenario at least once
cy.mount(SignalsOptionalComponent, {
componentProperties: {
title: 'Prop Title',
count: 7,
// @ts-expect-error
countChange: cy.spy().as('countChange')
},
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.
for signal()
s you need to create the output spy since we need to subscribe to the signal itself to propagate the event. The output isn't captured automatically unfortunately
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.
It would be great to test both scenarios (to and from) using the RxJs Interop with signals. Though it is in Developer Preview it is a key piece for most Angular devs.
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.
We should be OK from this perspective since we are passing a raw signal down. The interop package can't be used outside of the angular injection context, so I don't think this is going to be a popular use case at least within the cypress spec file
@@ -0,0 +1,536 @@ | |||
import 'zone.js' |
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.
We should do some validation around if including this is still necessary (I think it is). But we should also try to validate if mount
works in zoneless
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.
* @memberof MountConfig | ||
* @description flag defaulted to true to automatically detect changes in your components | ||
*/ | ||
autoDetectChanges?: boolean |
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.
We probably don't need this anymore with signals
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.
we still need it for the fixture update from what I can tell
npm/angular-signals/src/mount.ts
Outdated
// update the model signal with the properties updates | ||
toObservable(propValue, { | ||
injector, | ||
}).subscribe((value) => { |
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.
We should validate their isn't a memory leak here and that we don't need to unsubscribe (or that it is unsubscribed automatically when we teardown before each test)
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.
This was a memory leak. The observable by the looks of it hangs around because the subscription is active. To fix this, I pushed the subscription into a list to clean up when we clean up the fixture. Updated and fixed in 12c7e36. Really good catch!
…signals-ct-harness
<body> | ||
<div data-cy-root></div> | ||
</body> | ||
</html> |
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.
</html> | |
</html> | |
cy.get('[data-cy="signals-required-component-title-display"]').should('contain.text', 'Signals Component as Primitive') | ||
}) | ||
|
||
// FIXME: we currently need to allow this as there isn't a great way to set input signals in a component due to how input signals are instantiated |
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.
Do we have an issue logged for this work?
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.
logged #29732
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.
added a comment too in 001813e
npm/angular-signals/src/mount.ts
Outdated
} | ||
} | ||
|
||
// currently not a great way to detect if a function is an InputSignal. When we discover a better way to detect a signal we should incorporate it here. |
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.
Do we want to log an issue to track on finding better ways to determine this stuff?
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.
We probably should. I will create one!
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.
logged #29731
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.
also updated the comments in 001813e
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.
PR looks good to me. Thanks for putting this together @AtofStryker
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.
Nice work @AtofStryker!
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Since signals introduced new methods/types to the API, we need to introduce a new test harness to Cypress in order to support signals. Right now we are calling it
cypress/angular-signals
. This will be readily available once this PR is merged in and cypress is released. In the future, we should be able to eventually merge this upstream intocypress/angular
with the next major version of cypress when we remove/deprecate support for angular 13-16. In other words, the new harness need should not be permanent.Documentation / Behavior
Typings issue
Mentioned in the issue is the following:
This is because in our standard
cypress/angular
mount
function, we expect a direct 1:1 mapping as the prop typesIn our new
cypress/angular-signals
harness, we need to make sure that a given generic, typeT
(or in the example given in the issue, astring
) , can be inferred by aninput()
,signal()
, ormodel()
signal.This way, specifying a
string
for anInputSignal<string>
is a completely valid type.Input Signal Behavior
Getting
input()
signals to work OOTB was a bit difficult, since we cannot create them outside of an angular context and there is no way to provide an injection context, even when setting an initial value. Because of this, Cypress handlesinput()
signals when being injected into a component from the cypress context the following way:input()
signal, but the value provided is a match of the generic type, we wrap the generic value in a writablesignal()
and merge that into the prop. In other words:is really
This allows us to make a signal and avoid the
ctx.title is not a function
error mentioned in #29264 while allowing the passing of primitives into the component to work.Change spy for inputs
Since the prop in question is an
input()
, we do not propagate changes totitleChange
or related output spies. In other words, this will not work:Which brings up the question, how can I change or assert on input updates/changes?
input()
signal, but the value provided is a match of the generic type wrapped in a signal, we merge the value as is to allow for one-way data binding (more on this below).Since
input()
in our case can also take aWritableSignal
, we can just pass asignal()
as a variable reference into the component and mutate the signal directly. This gives us the one-way binding we need to testinput()
signals outside the context of Angular.Model Signal Behavior
Since
model()
signals are writable signals, they have the ability to support two-way data binding. This means Cypress handlesmodel()
signals the following way:modal()
signal, but the value provided is a match of the generic type, we set the generic value in amodel()
and merge that into the prop. In other words:Change spy for models
Since the prop in question is an
model()
and is aWritableSignal
, we WILL propagate changes tocountChange
if the output spy is created, either like the example below or ifautoSpyOutputs: true
is configured. However, the typing forcountChange
is not supported since this is technically not a prop (@Output()
actually adds this as a prop which is not true in our case, even though the signal output is completely valid).However, since
count
is a primitive in this case and outside the angular context, we CANNOT support two-way data-binding to this variable. In other words, these will not work:Which brings up the question, how can I have two-way data-binding set up?
model()
signal, but the value provided is a match of the generic type wrapped in a signal, we set the initial value and set up two-way data binding (more on this below). This means you can achieve two way data-binding in your component tests like such:Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
? feat: first pass at adding angular signals documentation cypress-documentation#5841 (approved)type definitions
?