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

Rename onrender/onteardown to oncreate/ondestroy #40

Closed
aickin opened this issue Nov 29, 2016 · 12 comments · May be fixed by leonardoadame/svelte#13
Closed

Rename onrender/onteardown to oncreate/ondestroy #40

aickin opened this issue Nov 29, 2016 · 12 comments · May be fixed by leonardoadame/svelte#13

Comments

@aickin
Copy link

aickin commented Nov 29, 2016

This is a very subjective issue and probably deeply affected by my experience with React, but the name onrender was quite confusing to me. I presumed that onrender would be called every time the component's DOM changed (i.e. every time it is rendered to HTML) rather than just once when the component is added to the document. (By the way, I presume that it's only called when the component is added to the doc based on the code samples; the documentation does not say one way or another.)

Two suggestions:

  1. Change the documentation's "Lifecycle Hooks" section so that it says clearly exactly when the lifecycle hook functions are called.
  2. Consider renaming onrender to onsetup. In my experience, "setup" is more usually used as a counterpart to "teardown". Another possibility would be something like onaddtodocument and onremovefromdocument.

As I say, this is super subjective, but it was legitimately confusing to me while reading over code, even after I figured out that onrender is only called on setup.

Thanks so much!

@mrkishi
Copy link
Member

mrkishi commented Nov 29, 2016

In regards to 1, it'd also be nice to know if components can be rerendered/reconstructed/remounted manually, either in place or in a new target.

On 2, perhaps consider reusing names from other frameworks, whichever's aligned with svelte's implementation: Custom Elements v1's connect, Vue and React's mount, Angular's init, and so on.

@vampaz
Copy link

vampaz commented Nov 29, 2016

I would go with mount.
init - all is good, ready to mount.
mount - added to the DOM and initial render done.

@aickin
Copy link
Author

aickin commented Nov 29, 2016

I would go with mount.

I'd be fine with that as well, with the caveat that onteardown should then be changed to onunmount.

@maraisr
Copy link

maraisr commented Nov 30, 2016

Agreeing with you @aickin - its VERY important to have a clear and concise api, that can be consumed and understood at first glace. onteardown -> onsetup - or - onmount -> onunmount would be ammaazinggg!

Much like React's will / did ComponentUpdate lifecycles.

@Rich-Harris
Copy link
Member

What about oncreate and ondestroy? Unambiguous (you can only create or destroy something once), and they have a nice symmetry

@maraisr
Copy link

maraisr commented Nov 30, 2016

@Rich-Harris yeah mate - love it!!!

@aickin
Copy link
Author

aickin commented Nov 30, 2016

+1 for oncreate/ondestroy!

@wessberg
Copy link

Might I suggest following the Custom Elements v1 spec for the names of lifecycle hooks? I think that's most forward looking, even though not as concise.

onrender would become connectedCallback.
onteardown would become disconnectedCallback

@Rich-Harris Rich-Harris added this to the 2.x milestone Dec 10, 2016
@Rich-Harris Rich-Harris changed the title Consider renaming onrender to onsetup Rename onrender/onteardown to oncreate/ondestroy Jan 8, 2017
@rrag
Copy link

rrag commented Jan 8, 2017

my $0.02 when I first saw onteardown I was looking for onsetup, you know like JUnit, in java which has setUp() and tearDown(). it did not occur to me that onrender is actually oncreate, so in the absence of a compelling reason, I suggest using the familiar (at least to those coming from Java) onsetup, onteardown

@Ryuno-Ki
Copy link

Ryuno-Ki commented Jan 8, 2017

In Python's unittest you use setUp and tearDown functions as well.

@Swatinem
Copy link
Member

Swatinem commented Jan 8, 2017

I actually think react did a good job finding names for these lifecycle events:

  • componentDidMount
  • componentWillUnmount
  • (plus maybe all the update related lifecycle events)

Well those names are a bit verbose, I would maybe call them afterMount and beforeUnmount, which also make it clear that we have a valid dom elements that are also attached (mounted) to the real dom.

I think there is an edge case involving yield fragments with the current onrender hook that does not guarantee a mounted dom element currently. I will have to come up with a testcase for that.

@Rich-Harris
Copy link
Member

I've opened #316 which implements oncreate and ondestroy in a non-breaking way (so that people have time to update their components before it becomes a breaking change in V2).

Re alternative name suggestions, I'm wary of using the same names that web components and React use, partly because they're kinda ugly and tedious to type (and don't gel with all the other single-word-all-lowercase properties), but mostly because when you do that, you need to get the semantics exactly the same otherwise you just end up confusing people. For example, my understanding is that disconnectedCallback is called when a web component is temporarily removed from the document, whereas a Svelte component is either alive or dead, it doesn't exist in a kind of off-document limbo. In fact, the reason we're here is that React users reasonably expected 'render' to mean roughly the same thing in Svelte as in React. So I'm still in favour of oncreate and ondestroy.

Rich-Harris added a commit that referenced this issue Mar 1, 2017
Rich-Harris added a commit that referenced this issue Mar 1, 2017
Rich-Harris added a commit that referenced this issue Mar 1, 2017
[WIP] warn on onrender/onteardown, replace with oncreate/ondestroy
robinhouston added a commit to robinhouston/svelte.technology that referenced this issue Mar 31, 2017
The deprecated method onrender() is used in one of the examples.
Change to oncreate().

See sveltejs/svelte#40
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 a pull request may close this issue.

9 participants