Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Cannot warn or use the previous DNodes when a supported DNode type of undefined is returned. #598

Merged
merged 1 commit into from
Jun 26, 2017

Conversation

agubler
Copy link
Member

@agubler agubler commented Jun 26, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

It was fine to warn when undefined was not a supported DNode value but now that undefined is a supported type, we cannot warn or automatically replace with the previous value.

Resolves #585

@agubler agubler requested a review from kitsonk June 26, 2017 09:41
@kitsonk
Copy link
Member

kitsonk commented Jun 26, 2017

Is it a valid/legitimate use case that the original DNode was of a value, but the return type is actually undefined? Ergo, could it be done in cases where the previous !== undefined && current === undefined?

Tangental to this, but related, would it make sense at the decorator API to allowing passing of an optional argument that was something like observe: boolean which would always ignore what is returned (and would even have an overload of (callback: (...) => void, observe: true)?

I think just "observing" the render is a legitimate use case. You want to watch but you want to contract to not modify the render.

@agubler
Copy link
Member Author

agubler commented Jun 26, 2017

Is it a valid/legitimate use case that the original DNode was of a value, but the return type is actually undefined? Ergo, could it be done in cases where the previous !== undefined && current === undefined?

Okay, so I originally did this - however this would mean that if the afterRender legitimately returned undefined it would use the previous dNode value - this doesn't seem correct.

I think just "observing" the render is a legitimate use case. You want to watch but you want to contract to not modify the render.

Personally think that the afterRender is only there to work with the decorating the dNodes, not to add any side affect like storing state.

@kitsonk
Copy link
Member

kitsonk commented Jun 26, 2017

Personally think that the afterRender is only there to work with the decorating the dNodes, not to add any side affect like storing state.

👍 Ok, if we find a legitimate use case for just being able to get notified of when something renders, we can revisit. I will admit everything I have seen to date is some sort of hack for something we wanted to support in another way.

@agubler agubler merged commit 3989242 into dojo:master Jun 26, 2017
@dylans dylans added this to the 2017.06 milestone Jul 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants