Skip to content

Conversation

rainerhahnekamp
Copy link
Collaborator

No description provided.

@rainerhahnekamp rainerhahnekamp changed the title withResource` withResource Aug 21, 2025
Copy link
Collaborator

@michael-small michael-small left a comment

Choose a reason for hiding this comment

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

Immediate round of things I noticed, will do at least one more at a deeper level in a bit

@@ -0,0 +1,321 @@
//** Types for `withResource` */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This meant to be moved down to the types?

@michael-small michael-small added the enhancement New feature or request label Aug 22, 2025
@michael-small
Copy link
Collaborator

michael-small commented Aug 22, 2025

As an rxResource enthusiast, I would like at least one test that has it, even though I know this is generic to any valid resource type. Even if it was just a cheap copy of an existing one, like this:

edit: done

        it('can accept an rxResource', async () => {
          const UserStore = signalStore(
            { providedIn: 'root' },
            withState({ userId: 1 }),
            withResource((store) =>
              rxResource({
                params: store.userId,
                stream: ({ params: id }) => of(id + 1),
              }),
            ),
          );

          const userStore = TestBed.inject(UserStore);

          await wait();
          expect(userStore.value()).toBe(2);
        });

@michael-small
Copy link
Collaborator

Oh, I pulled down your fork and tried to push up that commit fb77141 and assumed it would just open a PR to your fork. Oops, I didn't realize I could write directly to your fork's branch. Hope that change is good, I'll be careful next time knowing that.

Copy link
Collaborator

@michael-small michael-small left a comment

Choose a reason for hiding this comment

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

Two things and it's good

  • Verify that the change I accidentally committed directly to this is good with you (build and test are fine)
  • Please add something like that one rxResource test example somewhere, if not literally just that example. I suppose if you didn't mind I could just push it up too, apparently.

Otherwise, between this time and the times I have looked at the draft PR in the other repo, looks good to me.

@rainerhahnekamp rainerhahnekamp merged commit fb60ea7 into angular-architects:main Aug 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants