Fix splattributes handling of type attribute. #1178
Closed
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
tomdale
approved these changes
Oct 16, 2020
pzuraq
approved these changes
Oct 16, 2020
|
Looks good, thanks for fixing this! |
|
Great commit message, thank you for providing the context! |
|
@rreckonerr want to review this PR and check if the test coverage here seems adequate compared to your reproduction? If you spotted anything else that should be incorporated, feel free to make suggestions here or rebase+retarget your PR against this one. We will also probably want to add similar tests in ember-source as well as part of landing this in Ember. |
|
@rreckonerr Does this look good to merge? |
|
Haven't worked with Ember for a while now, so my review is not needed. Thank you! |
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.
This PR resolves an issue where a
typeattribute, passed in a component invocation, would not correctly override thetypeattribute in a template when applied with...attributes.In the existing implementation, we special case the
typeattribute in two places. One is at compile time, when we reorder thesetAttributeopcodes to apply thetypeattribute last. However, this does not address dynamic cases that are not known at compile time, such as...attributesor legacy features such as Ember'sattributeBindings. A second special case exists in theComponentElementOperationsclass, which collects aggregate attribute values and applies them once all values are known.However, currently we do not disable the first special casing when the second will apply. This introduced a bug with
...attributes, which relies on order to determine attribute precedence. Because we always "moved" thetypeattribute to the last position, values specified in a template could never be overridden by the invocation.To fix this, we rely on the fact that these cases are mutually exclusive and only apply one or the other. At compile time, if we see
...attributes, we do not reorder thetypeattribute because we know it will be appropriately re-ordered at runtime.Closes emberjs/ember.js#18232