[BUGFIX beta] Keep rest positional parameters when nesting contextual components if needed. #13764
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
If you are wondering why so many tests, it is because the merging is done in several places. :) |
| @@ -40,6 +40,142 @@ moduleFor('@htmlbars Components test: closure components', class extends Renderi | |||
| this.assertText('Hodi Hodari'); | |||
| } | |||
|
|
|||
| ['@test keeps nested rest positional parameters if rendered with no positional parameters']() { | |||
There was a problem hiding this comment.
can you add GH#13742 in the test title?
There was a problem hiding this comment.
To all of them or just this one?
There was a problem hiding this comment.
By all of them I mean only those added in this PR.
There was a problem hiding this comment.
I add that to the two that made sense to me. Let me know if you want it added to the rest!
|
Chatted in slack with @Serabe about this, he is going to rebase to trigger a fresh CI run then we can land for 2.7. |
components if needed.
Steps to reproduce the bug before this patch:
1. Create a component with rest positional param (like `link-to`).
2. Create contextual component (`(component "link-to" "index")`).
3. Render it (`{{component (component "link-to" "index")}}`).
4. Realize the component did not receive `"index"` at all.
What was causing it?
====================
Because of how positional parameters work (both in components and contextual
components), it is needed to treat them at each level and then merge them.
Sadly, this was causing to overwrite the rest positional parameters because
when no parameters are passed, the attribute get an empty array nonetheless.
In the example above, assuming `LinkToComponent.positionalParams === 'params'`:
1. Contextual component in `2` gets its positional parameters processed and
receives the following named parameters `{ params: ['index'] }`.
2. When rendering, the `{{component}}` processes its own parameters getting
`{ params: [] }`. Then it merges in a `new EmptyObject()` these attributes
on top of the ones in the contextual component. (See what I did there?
I used the JS keyword `new` as an adjective! He he. Don't judge me, it is
pretty late in here)
Solving the issue
=================
This PR changes the implementation of `mergeInNewHash` to keep the original
positional parameters if the new ones are empty and the positional parameter is
of type rest.
Fixes emberjs#13742
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Steps to reproduce the bug before this patch:
link-to).(component "link-to" "index")).{{component (component "link-to" "index")}})."index"at all.What was causing it?
Because of how positional parameters work (both in components and contextual
components), it is needed to treat them at each level and then merge them.
Sadly, this was causing to overwrite the rest positional parameters because
when no parameters are passed, the attribute get an empty array nonetheless.
In the example above, assuming
LinkToComponent.positionalParams === 'params':2gets its positional parameters processed andreceives the following named parameters
{ params: ['index'] }.{{component}}processes its own parameters getting{ params: [] }. Then it merges in anew EmptyObject()these attributeson top of the ones in the contextual component. (See what I did there?
I used the JS keyword
newas an adjective! He he. Don't judge me, it ispretty late in here)
Solving the issue
This PR changes the implementation of
mergeInNewHashto keep the originalpositional parameters if the new ones are empty and the positional parameter is
of type rest.
Fixes #13742