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-resource): Initial implementation of createResource #282

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eneajaho
Copy link
Collaborator

This PR implements createResource and supports Promises only.

Open for discussion.

Highly inspired from https://docs.solidjs.com/reference/basic-reactivity/create-resource

Depends on: #281 for computedPrevious change.

@eneajaho eneajaho requested a review from nartc February 15, 2024 12:01
Copy link

nx-cloud bot commented Feb 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2e2d278. 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


🟥 Failed Commands
nx affected --target=lint --parallel=3
✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

});

function load(p: Promise<TValue> | undefined) {
if (p && isPromise(p)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (p && isPromise(p)) {
if (isPromise(p)) {

Comment on lines +180 to +184
if (trigger() !== previousTrigger()) {
state.set('refreshing');
} else {
state.set('pending');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (trigger() !== previousTrigger()) {
state.set('refreshing');
} else {
state.set('pending');
}
state.set(trigger() !== previousTrigger() ? 'refreshing' : 'pending');

const value = signal<TValue | undefined>(options.initialValue);
const error = signal<TError | undefined>(undefined);
const trigger = signal(0);
const state = signal<CreateResourceStatus>(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this can be renamed to 'status' thus matching the Type Parameter itself too?

@OmerGronich
Copy link

OmerGronich commented May 25, 2024

This is exactly what I was looking for! I spent the weekend implementing my own version of this (😅) and was introduced to this PR by @tomer953.

Quick question: How is resource.latest() different from simply calling resource.data()? I know in SolidJS it doesn't trigger transitions and Suspense, but we don't have that in Angular afaik. 🤔

@tomer953
Copy link
Contributor

Oh I need this... yesterday!
Awesome work.

I wonder why exactly its working with Promise and not Observables?

  • Observable are way more common in Angular
  • Most of the http calls is made by the HttpClient Module who return observable (also, applying interceptors)
  • wrapping with lastValueFrom(this.http.get('https://api.example.com/data')); will be extra boilerplate
  • built in cancellation

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.

None yet

5 participants