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

onstate / oncreate is not idiomatic #1765

Closed
niahoo opened this issue Oct 4, 2018 · 7 comments
Closed

onstate / oncreate is not idiomatic #1765

niahoo opened this issue Oct 4, 2018 · 7 comments

Comments

@niahoo
Copy link

niahoo commented Oct 4, 2018

Hi,

I assume that many developers use a very common pattern : reacting to changes. It seems odd that both onstate and onupdate fire once with the initial properties of a component.

I think it would be way more expected that onupdate fires only when an actual update of the component is made. The current implementation seems more like an aftercreate but event there I find the changed property meaningless. Or I am missing something useful there.

Having to check for if(previous) in each onupdate implementation seems cumbersome.

Thank you.

@gka
Copy link
Contributor

gka commented Oct 7, 2018

I have definitely written hundreds of these if (previous) checks before...

@Rich-Harris
Copy link
Member

In many cases, onstate is used to do things like trigger network activity, and onupdate is used to e.g. measure elements and redraw a canvas, in which case you definitely do want them to trigger immediately.

If they did only run on subsequent changes, you'd have to jump through some extra hoops (extracting that code out into a method and calling it from both onstate/onupdate and oncreate, for example). Furthermore we'd have to introduce a brand new lifecycle hook for any code that should happen prior to the initial render (whereas you can currently use onstate for that).

So while it would make things easier in some cases to change the semantics of onstate and update, I think it would make other things a lot harder and increase overall complexity.

Anyway, there's an easy trick you can use that completely sidesteps the issue: instead of using the onstate and onupdate lifecycle hooks, use the (equivalent) on('state', ...) and on('update', ...) events:

export default {
  oncreate() {
    this.on('state', ({ changed, current, previous }) => {
      // this code will only run on subsequent updates
    });

    this.on('update', ({ changed, current, previous }) => {
      // this code will only run on subsequent updates
    });
  }
};

@niahoo
Copy link
Author

niahoo commented Oct 8, 2018

Thank you for the tip !

As you can see, I'm not saying that onstate should change.

Although, I'm saying that unless you like to have big chunks of code for different topics in your onupdate hook, you will use a method that you can also call from oncreate. And as Svelte is compiled, lifecycle hooks may be called only when declared.

Thank you.

@Rich-Harris
Copy link
Member

Perhaps this is more of a documentation issue then? Given the symmetry between onstate and onupdate I think it'd be odd if their behaviour diverged — perhaps we just need to emphasise that it's better to use the event form than the hook form for post-create updates.

@niahoo
Copy link
Author

niahoo commented Oct 12, 2018

Maybe a docs problem yes. Actuallay I don't see any difference between onstate and onupdate after the first onupdate is called. Is there any difference ?

@Conduitry
Copy link
Member

See https://svelte.technology/guide#lifecycle-hooks - it's a matter of whether the DOM has already been updated for that state. That could probably be made more prominent than comments in a block of code.

@niahoo
Copy link
Author

niahoo commented Oct 12, 2018

Yes it seems it is the only documented difference at this point. Kind of disappointing to have to check for if (previous) or fill oncreate with callbacks just to spare a hook that could be ignored (if not used) at compile time. But it will work.

Thank you

@niahoo niahoo closed this as completed Oct 12, 2018
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

No branches or pull requests

4 participants