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

[WIP] [BUGFIX beta] Support concatenatedProperties in contextual components #14956

Conversation

Serabe
Copy link
Member

@Serabe Serabe commented Feb 24, 2017

Given a component my-comp with a concatenatedProperties: ['foo'] this:

{{component (component (component "my-comp" foo=1) foo=2)}}

should produce foo: [1, 2] (unless there are other values in the class, then
those values would be concatenated as well).

This is not happening due to curryArgs does not take into account the
concatenatedProperties and mergedProperties properties. One of the problems
with current implementation is that these properties work at mixin level, thus
making them only accessible working out the proto (see emberjs/rfcs#209 for
more details.

This initial draft does not fully work. For this to continue I need to know
some details:

  1. Is there an implemented way in glimmer to create an Reference that is
    a collection of reference? I need to concatenate two references/array of
    references into an array.
  2. Is the proto method expensive enough not to continue?

Given a component `my-comp` with a `concatenatedProperties: ['foo']` this:

```hbs
{{component (component (component "my-comp" foo=1) foo=2)}}
```

should produce `foo: [1, 2]` (unless there are other values in the class, then
those values would be concatenated as well).

This is not happening due to `curryArgs` does not take into account the
`concatenatedProperties` and `mergedProperties` properties. One of the problems
with current implementation is that these properties work at mixin level, thus
making them only accessible working out the `proto` (see emberjs/rfcs#209 for
more details.

This initial draft does not fully work. For this to continue I need to know
some details:

1. Is there an implemented way in glimmer to create an Reference that is
   a collection of reference? I need to concatenate two references/array of
   references into an array.
2. Is the `proto` method expensive enough not to continue?
@homu
Copy link
Contributor

homu commented Mar 14, 2017

☔ The latest upstream changes (presumably #15014) made this pull request unmergeable. Please resolve the merge conflicts.

@Serabe Serabe closed this Jul 13, 2017
@AnastasiyaYelets
Copy link

@Serabe hey, why did this pull close? Issue still not fixed in ember

@Serabe
Copy link
Member Author

Serabe commented Mar 16, 2018

I can try to give this another try, the thing was supporting this is (or at least was) pretty expensive and nobody had requested it in over two years. We can try and give it another go.

@KirillSuhodolov
Copy link

@Serabe I founded similar issue #14914

I am trying to have merged properties that passing via component

{{component (component (component "my-comp" foo=(hash a=2)) foo=(hash b=3))}}

foo defined as merged property at component

looks at codebase it not implemented at ember-glimmer component helpers

@Serabe
Copy link
Member Author

Serabe commented Mar 16, 2018

Yes, I know. The problem with concatenatedProperties and mergedProperties is that they are instance properties, and therefore would require creating an instance of the component to know its value.

@KirillSuhodolov
Copy link

so, at such example will be create two extra waste components at runtime?

@Serabe
Copy link
Member Author

Serabe commented Mar 17, 2018

Yes.

@KirillSuhodolov
Copy link

@Serabe anyway, is it possible add to ember?

@Serabe
Copy link
Member Author

Serabe commented Mar 23, 2018

@AnastasiyaYelets @KirillSuhodolov this won't be fixed. This feature would be pretty expensive computationally and had some undesired side-effects from initialising the component each time.

Please, for anyone reading this, the behaviour that won't be fixed is not that concatenated properties won't work if a component is invoked as a contextual component, but that concatenated properties work in nested contextual components. Let me show this with an example:

{{! this won't concatenate properties }}

{{#with (component "my-component" classNames="first-declaration") as |comp|}}
  {{#with (component comp classNames="second-declaration") as |nestedComp|}}
    {{component nestedComp}}
  {{/with}}
{{/with}}

second-declaration is overwriting first-declaration as value. This ember twiddle shows the behaviour with a few classes.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants