Skip to content

fix resource state#1315

Merged
ryansolid merged 1 commit intosolidjs:mainfrom
mdynnl:resource-error
Oct 29, 2022
Merged

fix resource state#1315
ryansolid merged 1 commit intosolidjs:mainfrom
mdynnl:resource-error

Conversation

@mdynnl
Copy link
Contributor

@mdynnl mdynnl commented Oct 28, 2022

fixes #1309

@coveralls
Copy link

coveralls commented Oct 28, 2022

Pull Request Test Coverage Report for Build 3341902142

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.089%

Totals Coverage Status
Change from base Build 3287627654: 0.0%
Covered Lines: 1276
Relevant Lines: 1362

💛 - Coveralls

@ryansolid
Copy link
Member

Looking at the fix I'm worried something more serious is going on. These are in a runUpdate batch so they should run together and the order not matter. @mdynnl can you speak to this at all?

@mdynnl
Copy link
Contributor Author

mdynnl commented Oct 28, 2022

The write order actually affects the order updates run. It wouldn't matter if res is accessed before state (res.loading) in the same computation stopping further execution. But if state also has other computations that doesn't depend on res, this would allow them to run first. (Edit: I'm not entirely sure if this should be allowed)

Also, onError can be used if this is desired. ErrorBoundary would also work but res.loading has to be read outside of it.

@ryansolid ryansolid merged commit 454ed51 into solidjs:main Oct 29, 2022
@mdynnl mdynnl deleted the resource-error branch April 30, 2023 09:59
lilybw pushed a commit to lilybw/solid that referenced this pull request Nov 12, 2025
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.

Resource's error won't be set if used in createRenderEffect

3 participants