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

feat: signals test cases #3962

Merged
merged 11 commits into from
Feb 6, 2024
Merged

feat: signals test cases #3962

merged 11 commits into from
Feb 6, 2024

Conversation

jmsjtu
Copy link
Member

@jmsjtu jmsjtu commented Jan 27, 2024

Details

This is a draft to discuss the expected use cases for signals in LWC.

The implementation is based on the signals RFC with the following deviations:

  1. The LWC engine will attempt to subscribe to the signal when it has been bound to an LWC class and when it is used on a template.
  2. The signal does not need to be fully referenced on the template for the engine to subscribe, it could be used inside a getter for example.

Branch with the playground updated for signals.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

GUS work item

W-14560945

@jmsjtu jmsjtu added the nomerge label Jan 27, 2024
@jmsjtu jmsjtu requested a review from a team as a code owner January 27, 2024 02:34
@jmsjtu jmsjtu requested review from divmain and removed request for a team January 27, 2024 02:34
@jmsjtu jmsjtu marked this pull request as draft January 27, 2024 02:42

import { Signal } from './signal';

describe('signal protocol', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I created these tests as a basic sanity check for behavior we're expecting from a signals implementation.

The idea is for anyone planning to implement the interface to understand how the framework expects it to behave.

Comment on lines 8 to 9
describe('signal protocol', () => {
describe('lwc engine subscribes template re-render callback when signal is bound to an LWC and used on a template', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests validate when the framework will attempt to subscribe to a signal used in an LWC.

Comment on lines 20 to 21
describe('signal reaction in lwc', () => {
it('should render signal value', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests validate the content is displayed correctly after the framework has subscribed to a signal.

@jmsjtu jmsjtu marked this pull request as ready for review February 2, 2024 20:35
@jmsjtu jmsjtu requested a review from nolanlawson February 2, 2024 20:36
@jmsjtu jmsjtu removed the nomerge label Feb 2, 2024
Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

I know this is just the tests, but this already looks cool as hell! 🤘

.github/workflows/karma.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@divmain divmain left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. Thanks for splitting this out into a separate PR; it made everything much easier to review.

I'm guessing there are test cases that are missed here. But I'd recommend not spending any more time enumerating the cases. We'll have a better idea of the edge cases as the larger effort moves forward, and we can circle back to add tests as needed.

import ExplicitSubscribe from 'x/explicitSubscribe';
import List from 'x/list';

import { Signal } from 'x/signal';
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little weird to use LWC module resolution for this. I see why you're doing it, because to make it easier to import elsewhere in the test collateral. Maybe just leave a comment here, since at first glance it looks like you're importing a component named x-signal.

@jmsjtu jmsjtu merged commit 3c78f17 into jtu/signals Feb 6, 2024
1 of 3 checks passed
@jmsjtu jmsjtu deleted the jtu/signals-test-cases branch February 6, 2024 22:08
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.

3 participants