QwikDev / qwik-image

Qwik Image Component
MIT License
53 stars 5 forks source link

fix: image reactivity #3

Closed DustinJSilk closed 1 year ago

DustinJSilk commented 1 year ago

What is it?

Description

Reactivity is not currently build into the component as values are defined on the component rather than in signals with tasks. This PR switches to using useComputed$ which automatically tracks changes to dependencies without needing to add a subsequent useTask$. Props can now be updated and the image component will correctly update.

Unfortunately, it's not possible to run this project as the npm scripts are empty, the contributing guide seems out of date, and I don't have experience with NX so i cant run it. The test scripts also don't exist.

I hope this PR means we can update the contributing guide so it works correctly, or if one of the maintainers could test and run this PR. Alternatively please feel free to reach out to me to get this change over the line

This attempts to fix https://github.com/qwikifiers/qwik-image/issues/2

Use cases and why

Screenshots/Demo

Checklist:

nx-cloud[bot] commented 1 year ago

โ˜๏ธ Nx Cloud Report

Attention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here.

We didn't find any information for the current pull request with the commit e121f8d8d506509106bb9c3fea3edcadf85d216c. You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with ๐Ÿ’Œ from NxCloud.

gioboa commented 1 year ago

Thanks for this amazing PR. You can test it by running the application under /app folder qwik-demo-app With Nx vscode extension you can run the application easily I can't test it but you can test it and then I will merge it ๐Ÿ‘

gioboa commented 1 year ago

Great ๐Ÿ‘ is it ready to merge?

DustinJSilk commented 1 year ago

thanks @gioboa , I've added some unit tests that are passing now before and after these changes and I've also tested the demo app in the browser which seems to work correctly.

There is one issue however, eslint shows an error for qwik/valid-lexical-scope because it cannot serialize any of the img props such as onLoad$, onError$ etc which are function callbacks. This still works as I believe qwik automatically converts these to QRLs, so it might be an issue in Qwiks eslint plugin. I will try and open an issue there soon.

DustinJSilk commented 1 year ago

I have created an issue https://github.com/BuilderIO/qwik/issues/4553 to update the qwik eslint plugin or for some guidance from the qwik team on this warning.

gioboa commented 1 year ago

Thanks for you amazing work. I will merge the PR and I will release the new version of the library