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

attrs not updating correctly for contextual components #12613

Closed
amk221 opened this issue Nov 15, 2015 · 15 comments
Closed

attrs not updating correctly for contextual components #12613

amk221 opened this issue Nov 15, 2015 · 15 comments
Labels

Comments

@amk221
Copy link
Contributor

amk221 commented Nov 15, 2015

Please see the console on this twiddle

Edit: Sorry, badly named issue. Not sure what the actual problem is just yet.

@amk221 amk221 changed the title didReceiveAttrs not firing with contextual compeontns didReceiveAttrs not firing with contextual components Nov 15, 2015
@amk221 amk221 changed the title didReceiveAttrs not firing with contextual components attrs not updating correctly for contextual components Nov 15, 2015
@rwjblue rwjblue added the Bug label Nov 15, 2015
@rwjblue
Copy link
Member

rwjblue commented Nov 15, 2015

Haven't dug into exactly what is happening just yet, but this is definitely a bug.

@Serabe
Copy link
Member

Serabe commented Nov 16, 2015

While working on #12617 I'm also taking a look at this since both seem to touch the same classes.

I have notice that attrs is different in each case on rerender. It contains the right value in my-component but not in my-nested-component. I need to see how the closure component relates to the rerender.

@Serabe
Copy link
Member

Serabe commented Nov 19, 2015

I think I know what is happening. I'll check tonight (your this morning)

@Serabe
Copy link
Member

Serabe commented Nov 19, 2015

Submitted a PR fixing this. The shortcat for rerendering was being done to soon preventing the arguments from the closure component to be passed to state.manager.rerender.

Serabe added a commit to Serabe/ember.js that referenced this issue Nov 19, 2015
Rerendering a dot-syntax closure component failed to update the attributes due
to the shortcut being done too soon.

Fix emberjs#12613
@amk221
Copy link
Contributor Author

amk221 commented Nov 19, 2015

@Serabe Thanks for your work on the fix. Would you be able to look at the updated twiddle. I've tried to clarify what I am expecting the behaviour to actually be (see the console).

Essentially, I think that didReceiveAttrs is not firing with the correct value. But I might be expecting the wrong thing.

@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2015

Reopening for review.

@rwjblue rwjblue reopened this Nov 19, 2015
@Serabe
Copy link
Member

Serabe commented Nov 20, 2015

I think when Ember Twiddle updates with the fix it should be ok. @rwjblue do you know if there is a way to test a twiddle with current master?

@rwjblue
Copy link
Member

rwjblue commented Nov 20, 2015

https://ember-twiddle.com/5a29d14617527134ecad?numColumns=1 seems to be working properly (and using the correct SHA of Ember canary).

@amk221 - Can you confirm?

@amk221
Copy link
Contributor Author

amk221 commented Nov 20, 2015

Confirm, it's working. Thanks!
(Apologies, miscommunication between myself and rwjblue on Slack)

@amk221 amk221 closed this as completed Nov 20, 2015
@amk221
Copy link
Contributor Author

amk221 commented Nov 22, 2015

The RFC suggests I should be able to send along additional attributes to contextual components.

If you add foo='bar' as an attr to the contextual component in the twiddle, then the 'additional attribute' is not additional, but seems to wipe out the closed over attributes.

@Serabe
Copy link
Member

Serabe commented Nov 22, 2015

Please, can you send the updated tweedle? I did this and seems to work.

@amk221
Copy link
Contributor Author

amk221 commented Nov 22, 2015

@Serabe Yup, that's what I did too. Notice the difference in the console when you add that extra attribute? myProp does not increment, but it should?

@Serabe
Copy link
Member

Serabe commented Nov 22, 2015

I see now. Proper support for glimmer components are still in the making. I'll take this into account. Thank you.

@amk221
Copy link
Contributor Author

amk221 commented Nov 22, 2015

Ok thanks, this blocks a feature I need to create a select box component - is there a place I can track this issue?

@Serabe
Copy link
Member

Serabe commented Nov 22, 2015

The PR referenced above

rwjblue pushed a commit that referenced this issue Nov 29, 2015
Rerendering a dot-syntax closure component failed to update the attributes due
to the shortcut being done too soon.

Fix #12613

(cherry picked from commit ab72105)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants