-
Notifications
You must be signed in to change notification settings - Fork 535
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
Separation of concerns between IComponentHTMLView
and IComponentHTMLVisual
#1230
Separation of concerns between IComponentHTMLView
and IComponentHTMLVisual
#1230
Conversation
Opening this as a draft to give some insight into what the next change would potentially be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For back compat you might want to just leave the existing render interface in but note it's preferable to use something else. Or just move it to the loader so you can process the old interface.
* Components which wish to remove views from the DOM should call remove() on the view | ||
* before removing it from the DOM. | ||
*/ | ||
remove?(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy enough to just do nothing that we should make it required - just to avoid people missing the need for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see - you might just expose this (like the IComponentHTMLRender) - in which case remove doesn't make sense. Would be nice if in the addView case it was non-optional. Maybe they are separate interfaces but the view one extends the direct one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really matter whether the view was gotten directly or through addView
. I chose not to make it mandatory for two reasons:
- For most UI frameworks, it's optional to implement their equivalents of
remove()
(only needed if you have some mess to clean up like event listeners, timers, etc.). Making it optional here gives a closer parallel to those existing paradigms. - It's one less step for components converting from
Visual
toView
in response to these changes (ones that only implemented render() previously). This is a minor reason though; like you said it's easy enough to implement with a no-op.
It wouldn't be a big problem to make it mandatory, but my slight preference is to have it optional.
viewable.addView ? viewable.addView() : viewable; | ||
|
||
// First try to get it as a view | ||
let renderable = component.IComponentHTMLView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit of a back compat dance on this one - so you may need to keep the visual around - at least temporarily - but have the loader check for it last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what I'm doing already? Or maybe I'm misunderstanding?
…ew, and addView a mandatory member of IComponentHTMLVisual
Bumps [html-webpack-plugin](https://github.com/jantimon/html-webpack-plugin) from 5.6.2 to 5.6.3. - [Release notes](https://github.com/jantimon/html-webpack-plugin/releases) - [Changelog](https://github.com/jantimon/html-webpack-plugin/blob/main/CHANGELOG.md) - [Commits](jantimon/html-webpack-plugin@v5.6.2...v5.6.3) --- updated-dependencies: - dependency-name: html-webpack-plugin dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: WillieHabi <143546745+WillieHabi@users.noreply.github.com>
This change does a few things to improve separation of concerns between
IComponentHTMLView
andIComponentHTMLVisual
. After these changes,IComponentHTMLView
is the only interface you might call a "view", andIComponentHTMLVisual
is effectively a "view factory" only. All of this stems from the changes in componentRender.ts which is the best place to start:IComponentHTMLVisual
dropsrender()
, leaving onlyIComponentHTMLView
with therender()
. As such,IComponentHTMLRender
is no longer needed as a separate interface and is removed.render()
,IComponentHTMLVisual
is left as purely a view factory.addView()
becomes mandatory accordingly.remove()
optional accordingly. This parallels the optional cleanup methods in other UI frameworks (componentWillUnmount()
,beforeDestroy()
,ngOnDestroy()
).IProvideComponentHTMLView
is introduced, allowing components to be queried for whether they are renderable.Subsequently, the components in our repo change to match.
render()
areIComponentHTMLView
saddView()
areIComponentHTMLVisual
sIComponentHTMLVisual
components that implementedrender()
but would actually throw, no longer do that and should just be treated as a view factory.render()
can now be called with confidence that it won’t throw under normal conditions.IComponentHTMLVisual
components that implementedrender()
but would actually do something like.addView().render()
under the covers, no longer have arender()
and should just be treated as a view factory. As a result, we avoid leaked views (which cannot be correctly.remove()
d), as well as no more need fordefaultView
members.This change builds on top of #1079 but doesn’t strictly need to – webflow/flow-scroll could undergo the same set of changes as the other components in this change if we wanted to reverse order.