[BUGFIX beta] Create a new hash parameter when creating a component cell #12712

Serabe · 2015-12-14T20:41:20Z

If we merge positional and hash parameters without duplicating the hash before,
then we get an error when rerendering.

Fixes test in #12711

…omponent helper

rwjblue · 2015-12-14T20:47:13Z

packages/ember-htmlbars/lib/keywords/closure-component.js

@@ -74,12 +74,14 @@ export function isComponentCell(component) {
 }

 function createNestedClosureComponentCell(componentCell, params, hash) {
+  let newHash = assign(Object.create(null), hash);


Currently, we do not use inheritance in hash objects (AFAIK). Do we really need to introduce that here?

Also, if we do, it should use new EmptyObject() instead (import EmptyObject from 'ember-metal/empty_object').

I'll change it to new EmptyObject(). Thank you for the tip!

The thing is that processing positional params adds them to hash in place, therefore when component gets rerendered (or the component cell gets recomputed) the hash already has the value and the assertion fails. Other option would be to check that the param's name is not in the hash or, if it is, it is not exactly the same object.

I share @rwjblue hesitations about the prototype chain being the best way to resolve this.

mixonic · 2015-12-14T21:07:35Z

packages/ember-htmlbars/lib/utils/extract-positional-params.js


    assert(`You cannot specify both a positional param (at position ${i}) and the hash argument \`${positionalParams[i]}\`.`,
-           !(positionalParams[i] in attrs));
+           isActiveStreamParam || !(positionalParams[i] in attrs));


what is this assertion against active doing? it would skip all bound positional params?

My fault. I checked out the file forgetting about the previous commit.

mixonic · 2015-12-14T21:12:33Z

The test here only asserts that {{component (component "-looked-up" name greeting="Hodi")}} works. There isn't a potential conflict of name in the test at all. This patch seems to attempt to ignore any conflicts between hash and positional params during closure creation. They don't seem to be solving the same problem?

Serabe · 2015-12-14T21:33:58Z

I'll try to explain this better. The problem is that processPositionalParams changes the input argument hash in place. The problem is that when creating a component closure, positional parameters get merged into the hash. This needs to be done at each level of closure nesting to avoid false assertions being raised.
If the positional parameter was bound, when its value is changed, the closure component cell gets recomputed. This causes the positional params to be merged into hash, which already was done in creation.

There are other options:

Assertion is not raised if the value in params and the value in attributes is exactly the same. This can cause some assertion not being raised, like in the case {{my-component myName name=myName}} (the positional parameters is ['name']).
Closure component cell is not dependant on anything. This might work now, because we only nest streams and not much more, but if some spread operator gets into handlebars, this can cause problems.
Remove the assertion that was introduced when contextual components where introduced and choose a strategy of merging.

rwjblue · 2015-12-14T21:50:24Z

@Serabe - Thanks for the more detailed write up.

Would it be possible to allow disabling the assertion after the first render?

Serabe · 2015-12-14T21:53:26Z

I can try. Is there any way inside the compute method of a stream to know if it is a recompute?

Serabe · 2015-12-14T23:28:25Z

I did something, but I still don't fully like the solution. Another solution that might work is separating the processing from the assertions. Then, we can run the assertion at each cell component level and keep the positional parameters without processing them. Finally, the processing would be done normally, as any other component.

Serabe · 2015-12-14T23:29:05Z

I added a WIP until a satisfactory solution is found.

rwjblue · 2015-12-15T00:41:54Z

The current state seems better to me than adding prototypal inheritance to the hash, but it is unfortunate to have to thread that extra argument through so much.

Another solution that might work is separating the processing from the assertions. Then, we can run the assertion at each cell component level and keep the positional parameters without processing them.

This sounds interesting to me. That way we could just process the assertions when the cell is created, right?

Serabe · 2015-12-15T00:43:45Z

Yes. OTOH, for normal component we would go through the params twice (or we get duplicated code).

Serabe · 2015-12-15T00:46:44Z

Furthermore, if we go through the separation, we need to create new arrays and hashes when nesting for the merge. Otherwise we get a bug when nesting the same closure in two different places.

Serabe · 2015-12-16T00:34:36Z

I don't see many options right now except for to copy the attributes to a new object when creating the closure.

See this tweedle.

Long story sort: Ember processes positional params into the attributes, but does so in place. The components being rendered depend on this. I suggest doing this quick fix (copying the attributes inside the closure) and start looking at how to change the render process so processing positional params does not happen in place.

mixonic · 2015-12-18T21:37:38Z

@Serabe Yes, I agree. Building a new object is the right model- we should do that, then revisit for optimizations later on.

If we merge positional and hash parameters without duplicating the hash before, then we get an error when rerendering. Fixes test in emberjs#12711

Serabe · 2015-12-20T13:33:22Z

Done. I need to tackle a different issue on parameters before starting to work on optimizations.

[BUGFIX beta] Create a new hash parameter when creating a component cell

rwjblue · 2015-12-21T00:12:04Z

Thanks @Serabe!

[WIP][BUGFIX Beta] Positional param clash on rerender of contextual c…

07ea189

…omponent helper

rwjblue reviewed Dec 14, 2015
View reviewed changes

Serabe force-pushed the fix/positional-params-on-rerender branch from 3f6fc7a to d979491 Compare December 14, 2015 21:02

mixonic reviewed Dec 14, 2015
View reviewed changes

Serabe force-pushed the fix/positional-params-on-rerender branch from d979491 to d41021f Compare December 14, 2015 21:14

Serabe force-pushed the fix/positional-params-on-rerender branch from d41021f to 7c52e37 Compare December 14, 2015 23:21

Serabe changed the title ~~[BUGFIX beta] Create a new hash parameter when creating a component cell~~ [WIP] [BUGFIX beta] Create a new hash parameter when creating a component cell Dec 14, 2015

[BUGFIX beta] Create a new hash parameter when creating a component cell

4e7fe48

If we merge positional and hash parameters without duplicating the hash before, then we get an error when rerendering. Fixes test in emberjs#12711

Serabe force-pushed the fix/positional-params-on-rerender branch from 7c52e37 to 4e7fe48 Compare December 20, 2015 13:26

Serabe changed the title ~~[WIP] [BUGFIX beta] Create a new hash parameter when creating a component cell~~ [BUGFIX beta] Create a new hash parameter when creating a component cell Dec 20, 2015

rwjblue added a commit that referenced this pull request Dec 21, 2015

Merge pull request #12712 from Serabe/fix/positional-params-on-rerender

c3a8dcd

[BUGFIX beta] Create a new hash parameter when creating a component cell

rwjblue merged commit c3a8dcd into emberjs:master Dec 21, 2015

homu mentioned this pull request Dec 21, 2015

[WIP] [BUGFIX canary] Allow contextual components to be a glimmer component #12617

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX beta] Create a new hash parameter when creating a component cell #12712

[BUGFIX beta] Create a new hash parameter when creating a component cell #12712

Serabe commented Dec 14, 2015

rwjblue Dec 14, 2015

Serabe Dec 14, 2015

mixonic Dec 14, 2015

mixonic Dec 14, 2015

Serabe Dec 14, 2015

mixonic commented Dec 14, 2015

Serabe commented Dec 14, 2015

rwjblue commented Dec 14, 2015

Serabe commented Dec 14, 2015

Serabe commented Dec 14, 2015

Serabe commented Dec 14, 2015

rwjblue commented Dec 15, 2015

Serabe commented Dec 15, 2015

Serabe commented Dec 15, 2015

Serabe commented Dec 16, 2015

mixonic commented Dec 18, 2015

Serabe commented Dec 20, 2015

rwjblue commented Dec 21, 2015

[BUGFIX beta] Create a new hash parameter when creating a component cell #12712

[BUGFIX beta] Create a new hash parameter when creating a component cell #12712

Conversation

Serabe commented Dec 14, 2015

rwjblue Dec 14, 2015

Choose a reason for hiding this comment

Serabe Dec 14, 2015

Choose a reason for hiding this comment

mixonic Dec 14, 2015

Choose a reason for hiding this comment

mixonic Dec 14, 2015

Choose a reason for hiding this comment

Serabe Dec 14, 2015

Choose a reason for hiding this comment

mixonic commented Dec 14, 2015

Serabe commented Dec 14, 2015

rwjblue commented Dec 14, 2015

Serabe commented Dec 14, 2015

Serabe commented Dec 14, 2015

Serabe commented Dec 14, 2015

rwjblue commented Dec 15, 2015

Serabe commented Dec 15, 2015

Serabe commented Dec 15, 2015

Serabe commented Dec 16, 2015

mixonic commented Dec 18, 2015

Serabe commented Dec 20, 2015

rwjblue commented Dec 21, 2015