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

Update App.svelte by destructuring of an Object #258

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RajuPedda
Copy link

Destructured the response object to have a better comparison with other frameworks / libraries as the others also does the same destructuring.

Destructured the response object to have a better comparison with other frameworks / libraries as the others also does the same destructuring.
@RajuPedda
Copy link
Author

@matschik @pablo-abc Would you mind to review the changes please?

@pablo-abc
Copy link
Contributor

In Svelte 5 destructuring this is not possible since it would lose reactivity. The value of the property is computed on destructure rather than on access.

@RajuPedda
Copy link
Author

Yeah, I just test the code on Svelte 5 preview and your right, it loses the reactive nature if you destructuring the state object. Hence closing the PR.

@RajuPedda RajuPedda closed this Sep 26, 2024
@RajuPedda RajuPedda reopened this Sep 26, 2024
Used $derived state to get the reactive values.
@RajuPedda
Copy link
Author

Used the $derived state to get the reactive values and it's working now. Would you mind to review the changes please? @pablo-abc
Using $derived state

@pablo-abc
Copy link
Contributor

pablo-abc commented Sep 26, 2024

I haven't dug that much into how runes work but I'm quite surprised that works :p seems to be expected behaviour, though?

I see some pitfalls mentioned with doing it this way, though. There's two solutions at the end of that issue that seem slightly better?

E.g. without modifying useFetchUsers

const response = useFetchUsers();
const { isLoading, errors, users } = $derived(response);

Although I see no harm in this specific scenario... may be ok already. I'm not sure what you think, @matschik. Final decision is yours in the end 😄


On an unrelated note, I know it has nothing to do with your PR but I don't see why the example is using a class UserResponse to handle the state when something like this could do:

export default function useFetchUsers() {
  let users = $state();
  let error = $state();
  let isLoading = $state(false);

  async function fetchData() {
    isLoading = true;
    try {
      const response = await fetch("https://randomuser.me/api/?results=3");
      users = (await response.json()).results;
      error = undefined;
    } catch (err) {
      error = err;
      users = undefined;
    }
    isLoading = false;
  }
  fetchData();

  return {
    get users() { return users },
    get error() { return error },
    get isLoading() { return isLoading },
  };
}

Updated the code with suggested review changes
Removed extra spaces.
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.

2 participants