Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Add onAttach and onDetach lifecycle hooks #771

Merged
merged 8 commits into from
Nov 15, 2017

Conversation

agubler
Copy link
Member

@agubler agubler commented Nov 13, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Adds two lifecycle methods onAttach and onDetach for when a widget has attached and when it is detached.

Resolves #766

@agubler agubler requested review from kitsonk and matt-gadd November 13, 2017 19:43
@@ -169,6 +175,14 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> implement
// Do nothing by default.
}

protected onAttach(): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thought to this just being an optional method on WidgetBaseInterface?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, you will essentially always do a noop on most widgets on attachment, and I don't know the performance impact, but invoking potentially hundreds of the same function seems 😕 where as testing for presence and only calling if present feels like it would be more efficient...

Copy link
Member Author

@agubler agubler Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do the same for the onElementCreated / onElementUpdated, these guys get called on requestIdleCallback so certainly should reduce/negate the performance impact - but I can look at the optional method.

Copy link
Member Author

@agubler agubler Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is I don't want to put it on the interface (it's protected), so I don't think we can type it as such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, optional abstract isn't allowed and protected cannot be enforced on an interface... You ruin all my best ideas... 😝

@agubler agubler merged commit ffc2cdc into dojo:master Nov 15, 2017
@dylans dylans added this to the beta.5 milestone Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants