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

Add failing test demonstrating an issue updating a component property from didReceiveAttrs in test mode #17266

Closed
wants to merge 1 commit into from
Closed

Add failing test demonstrating an issue updating a component property from didReceiveAttrs in test mode #17266

wants to merge 1 commit into from

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Dec 6, 2018

This adds a failing test demonstrating an issue that I came across in our app when upgrading from 3.1 to 3.2 (the issue appears to be in 3.2 -> 3.5 and canary). The test added in this PR fails with a backtracking error:

screen shot 2018-12-06 at 10 24 30

I've also reproduced the issue in this 3.5 ember app. The error only occurs when testing the component, it works fine when running the app in development mode:

screen shot 2018-12-06 at 10 02 29

screen shot 2018-12-06 at 10 02 22

@GavinJoyce GavinJoyce changed the title Add failing test when updating a component property from didReceiveAttrs Add failing test demonstrating an issue updating a component property from didReceiveAttrs in test mode Dec 6, 2018
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This does seem like backflow to me (setting inside the components didReceiveAttrs flows back upwards to the invocation / callsite)...

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Dec 7, 2018

@rwjblue that passing a property is considered an invocation is a little surprising to me, but that's possibly caused by mental model not being correct.

{{foo-bar name=name}}

I can clearly see why this would cause a backtracking error:

hi {{name}}
{{foo-bar name=name}}

(I've confirmed that ^ raises a development time backtracking exception as expected)

Either way, there seems to be a difference between test and development modes that ideally we should resolve. There is no backtracking error when running in development but there is when running in test mode (since 3.2). (see comment below)

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Dec 7, 2018

Sorry, I'm mistaken about {{foo-bar name=name}} not raising a backtracking assertion in dev mode in 3.5, it does so the test failure in this PR is consistent with the development runtime error:

3.5:

screen shot 2018-12-07 at 10 30 14

3.1:

screen shot 2018-12-07 at 10 30 51

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Dec 7, 2018

This PR seems to have changed the timing of when didReceiveAttrs is fired, it used to fire in POST_INIT during component instantiation but now it fires in the component manager after the component has been instantiated.

@rwjblue now that we can see the source of the change in behaviour, what are your thoughts on whether this side-effect was intended or not? I'm happy to close this and fix the issue in my app of course, I just want to be sure that this behaviour is intended. If we don't believe that this should be a backtracking error, it might be possible to adjust how we track backtracking so that it works with the new timing

@rwjblue
Copy link
Member

rwjblue commented May 8, 2020

Sorry to have taken so long to reply here. Moving the invocation of didReceiveAttrs into the component manager (and out of the constructor flow) was specifically to help us ensure that auto-tracking things work properly (as well as remove more special behaviors around component constructors).

It is unfortunate that this bit y'all, but I think asserting in this context is correct behavior. Sorry again for the long delay in replying.

@rwjblue rwjblue closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants