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

Add IProvideComponentHTMLView interface as partial to IComponent #766

Closed
wants to merge 1 commit into from
Closed

Add IProvideComponentHTMLView interface as partial to IComponent #766

wants to merge 1 commit into from

Conversation

okjodom
Copy link

@okjodom okjodom commented Dec 11, 2019

Enable query and access of IComponentHTMLView interface on IComponent.

Use Case

IComponentHTMLView avails remove() utility called by component hosts to trigger proper disposal of nested component views

@okjodom okjodom changed the title add IProvideComponentHTMLView interface as partial to IComponent Add IProvideComponentHTMLView interface as partial to IComponent Dec 11, 2019
@okjodom okjodom marked this pull request as ready for review December 11, 2019 23:48
Copy link
Contributor

@leeviana leeviana left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -26,6 +26,10 @@ export interface IComponentHTMLView extends IComponentHTMLRender {
remove(): void;
}

export interface IProvideComponentHTMLView {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general i don't think this is the right direction, as it makes using these interfaces more complicated. It would require a check for IComponentHTMLView and IComponentHTMLVisual to figure out if something can render.

This also isn't the long term plan for these interfaces, so i'd rather not make changes in the runtime until we have a better long term plan. You should work with @ChumpChief on the pattern you are using to get unblocked in the short term, and provide longer term feedback

Copy link
Contributor

@skylerjokiel skylerjokiel left a comment

Choose a reason for hiding this comment

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

I agree with Tony. The only way you would currently have an IComponentHTMLView is if you call addView on IComponentHTMLVisual. In that case you should have typing because you know the return type.

Our goal currently is to minimize (only have 1) render interface so developers don't need to do multiple checks to see how to render the object.

@leeviana
Copy link
Contributor

I agree with Tony. The only way you would currently have an IComponentHTMLView is if you call addView on IComponentHTMLVisual. In that case you should have typing because you know the return type.

Our goal currently is to minimize (only have 1) render interface so developers don't need to do multiple checks to see how to render the object.

So then the question would be how can we call the remove() method that is only available via IComponentHTMLView right now?

@okjodom
Copy link
Author

okjodom commented Dec 21, 2019

I agree with Tony. The only way you would currently have an IComponentHTMLView is if you call addView on IComponentHTMLVisual. In that case you should have typing because you know the return type.
Our goal currently is to minimize (only have 1) render interface so developers don't need to do multiple checks to see how to render the object.

So then the question would be how can we call the remove() method that is only available via IComponentHTMLView right now?

Assuming we call render and addView once for every IComponentHTMLVisual, we may keep a reference to resulting IComponentHTMLView.

From current usage, render is only called once (from visual) and calling remove from any referenced view logically results in cleanup of visual rather than view. What then makes views from addView unique? Would it be more intuitive to have render and remove on a single interface?

@okjodom
Copy link
Author

okjodom commented Jan 17, 2020

@ChumpChief, @skylerjokiel Following up on this item, should we close PR and wait on interface redesign? Open an issue on the same?

@skylerjokiel
Copy link
Contributor

@JoDoM , we should close this. Matt is working on a redesign of these interfaces and either way this is not the intended use case for this interface.

@ChumpChief
Copy link
Contributor

Sorry, this fell off my radar over the holidays. Yes - we should close this.

@okjodom okjodom closed this Jan 17, 2020
jason-ha pushed a commit that referenced this pull request Jan 23, 2025
* version update for brainstorm and collaborative text area (#764)

* version update for brainstorm and collaborative text area

* remove azure-client from collaborative-text-area example, update brainstorm dependencies to use internal instead of dev

* Node demo and dice roller demo update to tinylicious 2.0 itnernal (#765)

* update dice roller to 2.0-internal tinylicious

* update node demo to tinylicious 2.0 internal

* V2.0 internal updates (#766)

* react-demo and react-starter-template

* audience demo and angular demo

* update FluidExamples 2.0.0-internal to latest internal version (#783)

* V2.0 internal update (#792)

* remove teams-fluid-hello-world example (#767)

* build: Misc. cleanup (#768)

* docs: Misc. README cleanup

* build: Update lockfile

* build: Update lockfile

* docs: Misc README cleanup

* docs: Update READMEs

* docs: Update README

* build: Update lockfile

* docs: Update README

* improvement: useMemo

* docs: Update README

* style: Sort package.json

* build: Update lockfile

* refactor: Update start flow to match other packages

* build: Update lockfile

* docs: Fix typo

* build: Update start flow to match other packages

* docs: Update README

* ci: Remove obsolete template

* fix: config

* style: Prettier

* style: Sort scripts

* style: Sort scripts

* revert: lockfile update

Seems to have broken tests

* brainstorm demo fix for remote azure example (#769)

* Add schedule to build apps daily (#771)

Add a schedule to automatically run builds on a daily basis

AB#4694

* docs: Clarify app usage instructions (#774)

Update app running instructions to be more consistent and explicit.

* tools: Update test infra to generate test reports locally and in CI (#775)

CI was set up to publish generated test reports, but none were being generated. This PR updates our testing logic to ensure test reports are correctly generated by test runs.

E.g.
![image](https://github.com/microsoft/FluidExamples/assets/54606601/4642657e-6e69-4f10-bba2-10ef5bd80a9c)

* Increase timeout for react-starter-template tests (#776)

This PR increases the timeout for react-starter-template tests, since they were consistently timing out in automated test pipelines.

* Bump @types/node to v16 (#777)

* Increase Timeouts (#780)

* Update root deps (#778)

Make syncpack a dev dep, update it and regenerate lock file.

* Update audience-demo (#782)

Update deps and improve readme for audience-demo.

Also updates CI from node 14 to node 16 to support ??= syntax.

These updates might help with the node update (#779 ), but with the unrelated non-deterministic CI errors and no useful error output, that's just a guess.

* Misc Updates and cleanup (#785)

Pull in updates from 61415de except for updating the node version, then npm install in all the examples.

This mainly updates a lot of deps to help enable node 18, and fixes many minor issues (updates to address deprecation warning from the new versions, fix alphabetical ordering, mode dev deps to dev deps etc.)

* Revert "Misc Updates and cleanup (#785)" (#786)

This reverts commit 11030fb.

* Un-revert dependency updates (#787)

Updates from #785, except for the build.yml change (which broke test running), and adding update to one jest version which caused a failing test.

* Update build.yml (#788)

Remaining changes from #785, cleaning up the build.yml. Includes a fix from that version which fixes test running.

This does not include the Node version update: that will follow in a separate change.

* Use node 18 on CI (#779)

Update to a supported version of NodeJs for CI.

* Use node 20 (#789)

Use and recommend (via nvm) Node 20.
New projects starting now should probably use Node 20, since it will be active LST and thus the best choice for production in just over a month.

Also, the Fluid project, as library authors, is granted the window of a Node release being "Current" (before it goes LTS) to ensure our libraries work: running these examples on the "current" release, Node 20, can help with that.

Support for node 18 will be kept (in engines and @types/node) for those who wish to use it, but it will not be tested by CI.

* Only support Node 18 and newer (#790)

Require at least Node 18.

This removes support for older unsupported major versions of NodeJs.

* version bump for brainstorm and collaborative text-area

* version bump for dice roller and audience demo

* version bump for react and node demo

* version bump react starter template

* angular demo bump

---------

Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Scott Norton <scottnorton@microsoft.com>
Co-authored-by: Sonali Deshpande <48232592+sonalideshpandemsft@users.noreply.github.com>
Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>

* Add v2.0_internal to yml file (#795)

* Bump to internal 7.1.0 (#796)

Also switches to test-runtime-utils in place of test-client-utils and bumps build-common to 2.0.2.

* bump to 2.0.0-internal.7.3.0 (#798)

Bump internal branch 2.0 packages to 2.0.0-internal.7.2.0

* bump package dependencies to 2.0.0-internal.7.2.2 (#800)

Co-authored-by: Michael Zhen <michaelzhen@WIN-1V7LUTHS292.redmond.corp.microsoft.com>

* bump FF package dependencies to 2.0.0-internal.7.3.0 (#803)

* Update dependencies to v8 internal (#804)

* Delete most demos

* Finish setting up directories

* Basic content swap

* Formatting

* Content swap, start prettier change

* Format

* Copyright headers

* Get basic tests working

* More permissive test timeouts

* Few more updates

* Update to rc

---------

Co-authored-by: Michael Zhen <112977307+zhenmichael@users.noreply.github.com>
Co-authored-by: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Scott Norton <scottnorton@microsoft.com>
Co-authored-by: Sonali Deshpande <48232592+sonalideshpandemsft@users.noreply.github.com>
Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
Co-authored-by: Michael Zhen <michaelzhen@WIN-1V7LUTHS292.redmond.corp.microsoft.com>
Co-authored-by: nmsimons <nick@dreamlarge.com>
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 this pull request may close these issues.

5 participants