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(create-notifier): add createNotifier #277

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

joshuamorony
Copy link
Contributor

This is an initial implementation for the createNotifier feature discussed here: #276

This incorporates some of the feedback in the discussion and we can continue to iterate on the exact API (personally I am happy with it as is but happy to change as well)

Once the exact implementation is settled I will add some docs to this PR (along with some more tests too)

Copy link

nx-cloud bot commented Feb 14, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8052943. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

@@ -0,0 +1,12 @@
import { signal } from '@angular/core';

export function createNotifier() {

Choose a reason for hiding this comment

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

createNotifier() is fine, but I'd give more context to the name like I mentioned on the discussion, like:

createManualSignalNotifier() or just createSignalNotifier().

Just a matter of preference though, createNotifier() is also fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to see what more people think but personally I find createManualSignalNotifier() too verbose - and I suppose if you do prefer being more explicit in the code you could still make the notifier names themselves more verbose e.g: myManualSignalNotifier = createNotifier()

import { signal } from '@angular/core';

export function createNotifier() {
const sourceSignal = signal(0);

Choose a reason for hiding this comment

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

Could think about something like, maybe:

const sourceSignal = signal<'FIRST_EMISSION' | number>('FIRST_EMISSION'); // or 'INIT'?

// Then on the notify() updater
return {
  notify: () => {
	  sourceSignal.update((v) => v === 'FIRST_EMISSION' ? 0 : (v+1)); // narrowing ftw :D
  },
  listen: sourceSignal.asReadonly(),
}

// In the effect, if we want to skip the first emission, the code is more readable this way and tells exactly what happened
effect(() => {
  if(myTrigger.listen() === 'FIRST_EMISSION') {
    return;
  }
});

Copy link
Contributor Author

@joshuamorony joshuamorony Feb 14, 2024

Choose a reason for hiding this comment

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

I also prefer the less verbose option here but it's a closer call for me here, in this case if(myTrigger.listen()) is a bit more mysterious. Again happy to wait and see what the general consensus is on this one, I don't mind it but I would probably prefer a slightly different approach. Maybe even having a separate computed signal e.g:

if(myTrigger.listenAfterInit()){
  // do stuff
}

^on second thought, downside of above is that it might seem like you can use it without the if e.g. just using myTrigger.listenAfterInit() - maybe doing an explicit check is the better option if we are going for more verbosity here

@joshuamorony joshuamorony marked this pull request as ready for review February 19, 2024 05:53
@eneajaho eneajaho self-requested a review February 20, 2024 09:15
Copy link
Collaborator

@eneajaho eneajaho left a comment

Choose a reason for hiding this comment

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

Love it!

@eneajaho eneajaho merged commit b1eff83 into ngxtension:main Feb 20, 2024
6 checks passed
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