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

.destroy() vs .teardown() for custom events #531

Closed
hville opened this issue Apr 27, 2017 · 3 comments
Closed

.destroy() vs .teardown() for custom events #531

hville opened this issue Apr 27, 2017 · 3 comments

Comments

@hville
Copy link
Contributor

hville commented Apr 27, 2017

in the custom methods documentation the event longpress returns {destroy} but the generated code in the REPL has longpress_handler.teardown() called in the destroy method. The longpress event would never get removed.

From #365 I understand component.destroy() is the retained convention and I am assuming the documentation is correct but the generated code is not?

(sorry if I am raising this as an issue instead of a PR but I am still trying to figure out the vision and conventions)

@Conduitry
Copy link
Member

I believe this is a docs issue, not a codegen issue. After teardown was renamed to destroy for components, the docs were updated (link), but the update was overenthusiastic in scope, as teardown was still used for custom event definitions (link), but I guess not all of the mistakes were caught.

That said, I'm not sure why custom event handlers do still use teardown. Because they're seen as something conceptually different than destroying a component? Or because of the technical difficulty of deprecating teardown while still keeping old code working? Either way, this also seems ripe for a runtime dev: true check and warning.

@Rich-Harris
Copy link
Member

Yeah, I think we should rename it to destroy, but for now the docs are just wrong. Have just pushed a fix. Will leave this open so that we can address the underlying issue. It'd be nice to deprecate this gracefully — remove teardown in v2, but support both until then with a dev warning.

For event handlers that are declared inline, like the one in the docs example, we could catch it at compile time in most cases.

@Rich-Harris
Copy link
Member

Fixed in 1.59

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

3 participants