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

fix: update onResolved for resource #7085

Open
wants to merge 4 commits into
base: build/v2
Choose a base branch
from

Conversation

gimonaa
Copy link
Contributor

@gimonaa gimonaa commented Nov 22, 2024

What is it?

new test to verify the proper update of the Resource component

  • Bug
  • tests

Description

I added a new test to verify the proper update of the Resource component.
When there is an input or an image inside a Resource component, it does not update correctly. However, if onPending is uncommented, the component updates as expected.

Screenshot 2024-11-22 alle 00 52 44

Screenshot 2024-11-22 alle 00 53 21

Checklist

  • My code follows the developer guidelines of this project
  • I performed a self-review of my own code
  • I added a changeset with pnpm change
  • I made corresponding changes to the Qwik docs
  • I added new tests to cover the fix / functionality

@gimonaa gimonaa requested a review from a team as a code owner November 22, 2024 05:36
Copy link

changeset-bot bot commented Nov 22, 2024

⚠️ No Changeset found

Latest commit: 4a27aa2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Varixo
Copy link
Member

Varixo commented Nov 22, 2024

Hey, are you working on a fix or just adding the failing test case?

@gimonaa
Copy link
Contributor Author

gimonaa commented Nov 22, 2024

Hey, are you working on a fix or just adding the failing test case?

Only test

@Varixo
Copy link
Member

Varixo commented Nov 29, 2024

I have fixed the JS part of this problem. There is the optimizer bug atm to solve.
The optimizer incorrectly marks the arguments of the onResolved function as constprops instead of varprops

@Varixo Varixo self-assigned this Nov 29, 2024
@Varixo Varixo changed the title fix: add v2 test - should update elements correctly fix: update onResolved for resource Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants