Skip to content

Add reference to the parent component in component's instance. #1539

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

Closed
PaulMaly opened this issue Jun 13, 2018 · 18 comments
Closed

Add reference to the parent component in component's instance. #1539

PaulMaly opened this issue Jun 13, 2018 · 18 comments

Comments

@PaulMaly
Copy link
Contributor

Seems, it'll be very nice, if we'll be able to have a reference to the parent component, e.g. this.parent just like we already have with a root, this.root.

Also, it could helps with #1538 issue.

@Rich-Harris
Copy link
Member

I've avoided adding this so far because I think that a component having knowledge of its parents is something of an anti-pattern — it makes it easy to tie yourself in knots instead of encouraging good design practices. Generally I think most things you'd use this.parent for can be done better with events and bindings.

Reasonable people can disagree on that; I would be interested to hear what other people think.

#1538 could be solved in other ways, e.g. explicitly passing this.store down to child components upon instantiation.

@PaulMaly
Copy link
Contributor Author

I've avoided adding this so far because I think that a component having knowledge of its parents is something of an anti-pattern — it makes it easy to tie yourself in knots instead of encouraging good design practices.

I understand that, but how about having knowledge about root? Seems it's also anti-pattern and I don't think that many people use it for regular things, but this gives us a choice to use it if we really need it.

I think parent could have the same value - everyone knows that it's a bad practice, but if business requirements or legacy architecture forces to use it, so, it can become a compromise from even more painful crutches.

#1538 could be solved in other ways, e.g. explicitly passing this.store down to child components upon instantiation.

Hm, I can't imagine that. Usually, I use components in declarative way (html-like tags). I'm not creating instances of sub-components manually. Could you please explain your idea?

@silentworks
Copy link

I agree with @Rich-Harris on this, but I also question passing down root to all components too. I think both are anti-patterns and should be avoided where possible.

@PaulMaly you can still pass the store to the html like tag, isn't the store just a data object anyway? you would pass it the same way you pass data to an html-like tag component.

@Rich-Harris
Copy link
Member

What I mean is that the code Svelte generates could pass the store down —

// code generated by Svelte
var nested = new Nested({
  root: component.root,
  store: this.store
});

In most cases this.store === this.root.store but in the cases we're concerned about, it would mean that options.store in the child would be equal to this.store in the parent. It's unfortunate that it'd mean generating extra code for each component, of course.

I take the point about root. I think that's an anti-pattern too, but since Svelte needs to store references to the root component for a variety of reasons, there isn't really a way to prevent it. People had started using this._root for various hacks, and having it as a documented part of the API seemed like the lesser of two evils!

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Jun 13, 2018

What I mean is that the code Svelte generates could pass the store down —

Ok, at least we don't need to do that manually.

In most cases this.store === this.root.store but in the cases we're concerned about, it would mean that options.store in the child would be equal to this.store in the parent. It's unfortunate that it'd mean generating extra code for each component, of course.

Actually, my concern becomes from my understanding of Svelte's Store idea - it was very clear for me that store inheriting for the hierarchy of components. In this case, component-specific stores should pass its store down to the hierarchy or we just need to fully deprecate this way.

Seems, some of your replies confirm this understanding:

While it is possible for any subtree to have its own store that makes it impossible for components in that subtree to access properties of the global store, which is often necessary.

But that code looks like it's design intent rather than a bug:

component.store = component.root.store || options.store;

I think, we should make a final decision how stores should works. Should it be a single root component store or subtree-stores also available. I prefer second variant, for me it's more flexible in future, but not breaks ability to use single store. Maybe I'm wrong.

Also, too little sense to have this.store === this.root.store since we already have this.root and it's inevitably. Then we always can get values from it manually. But subtree store could be very-very useful.

@silentworks

you can still pass the store to the html like tag, isn't the store just a data object anyway?

Nope, it's not a part of data object.

@arxpoetica
Copy link
Member

I vote no. In Ractive, this approach led to spaghetti code. The constraints force people to code in a more reactive way, imo.

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Jun 13, 2018

@arxpoetica I never used it in Ractive, because Ractive too powerful to use such dumb methods. Usually, It's not necessary at all.

Actually, I don't need to have this.parent itself. I just thought that it's simplest way to replace this code:

component.store = component.root.store || options.store;

to

component.store = component.parent.store || options.store;

If @Rich-Harris knows the better way to do that, so, move on, I'm not opposite that.

@btakita
Copy link
Contributor

btakita commented Jun 13, 2018

Having to keep track of instances (multiple stores) leads to incidental complexity. Having an singleton store removes this burden.

I'm happy working with the constraints of a singleton store. Even deeply nested data is accessible in svelte-extras.

When you work with a singleton store, you can see all of the page's state in one object. Redux calls this the "Single Source of Truth" principle. https://redux.js.org/introduction/three-principles. Note that I'm a fan of Redux's first principle, but don't agree with 2 & 3.

@PaulMaly
Copy link
Contributor Author

@btakita Having multiple stores per components hierarchy will not force you to use them. It still will be up to you. If you like to use redux-like single store attached to root component, you still be able to do that.

@btakita
Copy link
Contributor

btakita commented Jun 13, 2018

Having multiple stores per components hierarchy will not force you to use them.

I agree. I'd rather think of a Store as an aggregated data service instead of a Model object. That way, I don't need to worry about bookkeeping the instances since there's only one instance to be concerned about.

I have also passed instantiated store instances into a component (with mixins being applied to the instantiated store). It's, at times, convenient to do so. However, in doing so, I now have to track multiple instances & the pages' state is fragmented in multiple stores.

I'm in favor of treating instantiated stores as a property & adding methods to Store which facilitates communication between stores, though.

@PaulMaly
Copy link
Contributor Author

@btakita We already have a discussion about mixins and all that stuff in another issue. It definitely makes sense, but in my work, I usually use more than one source of data and don't like a concept of the single store from redux. It's my choice, and for me, the current implementation of Store is great in all matters except inheritance of the store.

I believe if a store will be inherited from the nearest parent component in the tree with a redefined store, it will give more advantages for me. I'll have a choice - use single root store with, for example, aggregation, or use multiple-stores on different levels in the hierarchy.

@stalkerg
Copy link
Contributor

I agree with @PaulMaly in some parts:

  1. component.store = component.root.store || options.store; 100% counterintuitive
  2. Without inheritance from a parent for the store will be difficult to make complicated svelte components for the market.

I suppose we can just send store to children in a hidden manner instead of introducing "parent" and getting store from his.

@btakita
Copy link
Contributor

btakita commented Jun 15, 2018

Without inheritance from a parent for the store will be difficult to make complicated svelte components for the market.

Mixins would be an even better candidate for making complicated svelte components for the market for a few reasons:

  1. All of the data is in a single store instead of being fragmented across mutliple stores. Note that svelte-extras has support for deeply nested data. Note that the "single source of truth" is a strength of Redux. There is also plenty of precedent of Repositories, Registries, Databases, acting as a data service vs the incidental complexity of having to manage ephemeral instances.
  2. Instead of having to traverse a store graph & manage instances of stores, one can simply reference a property in the singleton store
  3. A singleton store with globally scoped property names is a pattern that is usable today & does not require additional complexity in Svelte's api.
  4. Globally scoped property names also work across services. You can use the same names in all of the services of your stack, removing incidental mapping logic.

Inheritance hierarchies tend to be familiar, however inheritance is limited in it's applicability vs a composition-based approach.

@stalkerg
Copy link
Contributor

stalkerg commented Jun 15, 2018

Mixins would be an even better candidate for making complicated svelte components for the market

I think Mixins solve another problem. We need a solution for isolation tree of components includes his own store copy. I don't want to give access to the whole project for a component which installed by npm.

@PaulMaly
Copy link
Contributor Author

@stalkerg yep, read my mind

@PaulMaly
Copy link
Contributor Author

Closed with a related issue #1538

@paulocoghi
Copy link
Contributor

paulocoghi commented Jun 20, 2018

Now that #1552 is merged, is it a breaking change? For example, if I still want to use just a singleton store, as @btakita said, everything remains the same as informed on documentation, or we have to do something different?

@stalkerg
Copy link
Contributor

@paulocoghi in your case nothing change.

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

7 participants