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

Should component.mount be part of the public API? #150

Closed
Rich-Harris opened this issue Dec 7, 2016 · 7 comments
Closed

Should component.mount be part of the public API? #150

Rich-Harris opened this issue Dec 7, 2016 · 7 comments

Comments

@Rich-Harris
Copy link
Member

I actually missed this when it was introduced – slaps self on wrist for not paying close enough attention to #91 – but via #149 I just realised we now have a component.mount(node) method.

Obviously we need that for nested components, since mount happens at a separate time to initialisation, but should it be a public method (as opposed to e.g. an undocumented private _mount method)?

My instinct is that it shouldn't, because once you have mount, you kind of have to have unmount (which is different from teardown, but in a slightly confusing way), adding additional code to components that the vast majority of people will never use. And it invites additional onmount and onunmount lifecycle hooks, which muddy the API without that much benefit. Once these things make it into the design, you're basically stuck maintaining them (and explaining the distinction between render/mount and teardown/unmount to confused newcomers) forever.

@FWeinb
Copy link
Contributor

FWeinb commented Dec 7, 2016

In #149 mount is used exactly like you are describing it will be used but you are right that teardown isn't the way to remove it again so there will be issues. I vote for note making it public.

@Swatinem
Copy link
Member

Swatinem commented Dec 8, 2016

I think this should definitely be public api.

For example when integrating with other frameworks, or when you want completely custom components that render a yield fragment with custom code (for example to render it to completely different parts of the dom, similar to react portals).

Clarifying the lifecycles and distinction between teardown (destroy) and unmount a little better would be a plus.

@FWeinb
Copy link
Contributor

FWeinb commented Dec 8, 2016

But you can always render a component by instantiating it like:

import SomeComponent from './SomeComponent.html'
const comp = new SomeComponent({
   target: someNode
});

I don't see way the user needs to separate the creating and mount phase of a component?

@punmechanic
Copy link

Hey guys,

Note that my usage of mount was mainly due to the fact that I couldn't find a way in documentation to intuit the dynamic mounting of components (I thought target might be a "special" property and only used for the original bootstrapping of the app).

If this were more clearly delineated (i.e, in a "FAQ: How do I dynamically mount components?") then I'd see no issue with removing mount.

@Rich-Harris
Copy link
Member Author

There's nothing special about target – you'd basically do it like this:

  export default {
    onrender() {
      const RouteConstructor = this.get('route')
-      const currentComponent = new RouteConstructor({})
+      const currentComponent = new RouteConstructor({
+        target: this.refs.container
+      })

-      currentComponent.mount(this.refs.container)
-
      currentComponent.on('navigate', () => {
        console.log('test')
      })

For example when integrating with other frameworks, or when you want completely custom components that render a yield fragment with custom code (for example to render it to completely different parts of the dom, similar to react portals).

Bitter experience has taught me that it's much better to limit the API to what you need today and let people hack around the limitations (e.g. if you really wanted mount functionality you could render to an off-DOM element and insert/detach yourself) until the cost of resisting the API change outweighs the cost of maintaining something that you might not need. YAGNI, and all that. Being hyper-vigilant about these things pays dividends in the long run.

@punmechanic
Copy link

punmechanic commented Dec 8, 2016

There's nothing special about target – you'd basically do it like this:

Yeah, I tried to convey that I thought that in past tense - Preaching to the choir here! 😄

Rich-Harris added a commit that referenced this issue Dec 9, 2016
Rich-Harris added a commit that referenced this issue Dec 9, 2016
@Rich-Harris
Copy link
Member Author

I've just released 1.2.2 which removes mount (since it was never documented or tested it's not technically a breaking change...), per the general consensus in this thread. We can reinstate it later if needs be!

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

4 participants