Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

feat(core): Resources api change #60610

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

feat(core): Resources api change #60610

wants to merge 5 commits into from

Conversation

Humberd
Copy link
Member

@Humberd Humberd commented Mar 28, 2025 •
edited
Loading

  • fix(core): getting resource value throws an error instead of returning undefined
  • fix(core): narrow error type for resources API
  • fix(core): move reload method from Resource to WritableResource
  • build: use es2022 lib

@pullapprove pullapprove bot requested review from josephperrott and mmalerba March 28, 2025 16:54
@angular-robot angular-robot bot added detected: breaking change PR contains a commit with a breaking change detected: feature PR contains a feature commit area: build & ci Related the build and CI infrastructure of the project area: core Issues related to the framework runtime labels Mar 28, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 28, 2025
@Humberd Humberd requested a review from alxhub March 28, 2025 16:56
This was referenced Mar 28, 2025
Closed
Closed
Closed
@Humberd Humberd force-pushed the resources-api-change branch 2 times, most recently from ef2b989 to f5bb124 Compare March 28, 2025 17:30
@mmalerba mmalerba requested review from JeanMeche and removed request for mmalerba April 1, 2025 15:16
JeanMeche
JeanMeche reviewed Apr 1, 2025
View reviewed changes
packages/common/http/src/resource.ts Outdated Show resolved Hide resolved
josephperrott
josephperrott requested changes Apr 2, 2025
View reviewed changes
packages/tsconfig.json Outdated Show resolved Hide resolved
packages/tsconfig.json Outdated Show resolved Hide resolved
@Humberd Humberd force-pushed the resources-api-change branch 3 times, most recently from efd6d08 to 7825c04 Compare April 4, 2025 10:39
JeanMeche
JeanMeche reviewed Apr 4, 2025
View reviewed changes
packages/common/http/src/resource.ts Outdated Show resolved Hide resolved
devversion
devversion reviewed Apr 4, 2025
View reviewed changes
packages/tsconfig.json Show resolved Hide resolved
@atscott atscott modified the milestones: Backlog, v20 candidates Apr 4, 2025
@Humberd Humberd force-pushed the resources-api-change branch from 7825c04 to 2a9d55c Compare April 10, 2025 09:42
@Humberd Humberd force-pushed the resources-api-change branch from 2a9d55c to 0b9fe8a Compare April 17, 2025 12:43
@Humberd Humberd force-pushed the resources-api-change branch 3 times, most recently from 388387f to ad797ad Compare April 23, 2025 15:01
josephperrott
josephperrott approved these changes Apr 23, 2025
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@Humberd Humberd force-pushed the resources-api-change branch from ad797ad to ca83bd2 Compare April 23, 2025 15:18
@Humberd
Copy link
Member Author

Humberd commented Apr 23, 2025

After the approvals were done I changed the error message from: 'Resource failed to load' to `Resource is currently in the error state'.

I also removed untracking of the stream https://github.com/angular/angular/pull/60610/files#diff-16f6b7cc4c4e69b511c8312d38c6cf71c412e6f1d6ac9092054c0f349215cdedR418, because there were some cases in g3, where hasValue was out of sync with value signal and in result was throwing errors.

@alxhub alxhub force-pushed the resources-api-change branch from ca83bd2 to f788f52 Compare April 23, 2025 23:32
@Humberd Humberd force-pushed the resources-api-change branch from f788f52 to 886c7ce Compare April 24, 2025 12:33
pkozlowski-opensource
pkozlowski-opensource approved these changes Apr 25, 2025
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from devversion April 25, 2025 18:07
@devversion devversion removed their request for review April 29, 2025 11:33
@pullapprove pullapprove bot requested a review from devversion April 29, 2025 11:33
devversion
devversion approved these changes Apr 29, 2025
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@Humberd Humberd force-pushed the resources-api-change branch 3 times, most recently from ff4cfdc to 3161918 Compare April 30, 2025 09:40
@angular-robot angular-robot bot removed the detected: feature PR contains a feature commit label Apr 30, 2025
crisbeto
crisbeto approved these changes Apr 30, 2025
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@JeanMeche JeanMeche mentioned this pull request May 2, 2025
Closed
Humberd added 5 commits May 6, 2025 15:16
@Humberd
build: use es2022 lib
84bb207 
It was set to es2022 for build tsconfig in https://github.com/angular/angular/pull/60066/files#diff-7ec658aedc62a693cc513accc8f360938d90bcbdb09aa79c1401e56fe84f5fd3, but was missed for the main one.
@Humberd
fix(core): move reload method from Resource to WritableResource
48e317e 
Now only mutable resources can be reloaded

BREAKING CHANGE:
* `Resource.reload` has been moved to `WritableResource.reload`.
@Humberd
fix(core): narrow error type for resources API
3ac094a 
`Resource.error` used to return `unknown`. Now it's `Error | undefined`.
For non-`Error` types they are encapsulated with the `Error` type.

BREAKING CHANGE:
* `Resource.error` now has a type of `Signal<Error | undefined>` instead of `Signal<unknown>`
@Humberd
fix(core): getting resource value throws an error instead of returnin…
a0cd72f 
…g undefined

When there is an underlying error state it would not be possible to swallow the error with:
`computed(() => res.value()?.inner);`

BREAKING CHANGE:
* `Resource.value()` now throws an error when it's in an error state. Previously it returned `undefined`.
@Humberd
fix(core): reading resource value after reload in the error state
5e62b07 
When the resource is loading after reloading from the error state reading `Resource.value()` would return the default value instead of throwing an error.
This change prevents `Resource.hasValue()` from throwing an error in such a case.

BREAKING CHANGE:
* `Resource.value()` now returns a default value when in a loading state after reloading the error state
@Humberd Humberd force-pushed the resources-api-change branch from 3795382 to 5e62b07 Compare May 6, 2025 13:18
@mmalerba mmalerba removed their request for review May 7, 2025 13:59
@wartab
Copy link
Contributor

wartab commented May 13, 2025 •
edited
Loading

Since I was one of the people mentioning error typing during the RFC, I'm wondering about two things:

  • Why isn't the error part of the Resource generic à la Resource<T, E>?
  • Why do errors need to be instances of Error? This seems overly restrictive to me.

@alxhub alxhub mentioned this pull request May 16, 2025
Closed
@JeanMeche
Copy link
Member

Changes landed as part of #61441

@JeanMeche JeanMeche closed this Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanMeche JeanMeche JeanMeche left review comments

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@alxhub alxhub alxhub approved these changes

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion devversion approved these changes

@josephperrott josephperrott josephperrott approved these changes

Assignees
No one assigned
Labels
area: build & ci Related the build and CI infrastructure of the project area: core Issues related to the framework runtime detected: breaking change PR contains a commit with a breaking change
Projects
None yet
Milestone
v20 candidates
Development

Successfully merging this pull request may close these issues.
9 participants
@Humberd @wartab @JeanMeche @pkozlowski-opensource @alxhub @crisbeto @devversion @josephperrott @atscott
Morty Proxy This is a proxified and sanitized view of the page, visit original site.