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

Setting msg.enabled without msg.payload results in a TypeError #212

Closed
JaronrH opened this issue Apr 21, 2017 · 5 comments
Closed

Setting msg.enabled without msg.payload results in a TypeError #212

JaronrH opened this issue Apr 21, 2017 · 5 comments
Assignees
Labels

Comments

@JaronrH
Copy link

JaronrH commented Apr 21, 2017

Sending a component, such as the slider, a msg of just {enabled: false} results in a TypeError of "cannot read property 'toString' of undefined". This is due to the uiSlider trying to update the value via ui.js's toNumber() routine by accessing the non-existent property payload (line 48 at time of writing).

Ideally, I would think that you would want to look for msg.payload and only try to process it when it exists (if (msg.hasOwnProperty("payload")). That way, users could update the enabled/disabled state and/or the payload as they see fit as opposed to always having to pass in the current payload.

If it helps, my particular use case is that I have a slider that I want to make read-only by disabling it. I don't want to change the value though (in my case, a temperature) when I disable it.

@dceejay
Copy link
Member

dceejay commented Apr 22, 2017

co-incidentally this is actually similar to #211 - so the proposed fix of ignoring msg without any payload (but still handling msg.enabled) would fix both

@dceejay dceejay added this to Being worked on - or done but not on Npm in Dashboard - Things to do Apr 22, 2017
@dceejay
Copy link
Member

dceejay commented Apr 30, 2017

So - reviewing this change - the fix has broken a lot of other things (dropping msgs without payload was bad... as the text and template nodes could also render things other that payload...)

So thought now is to catch the undefined .toString() error - and b) return undefined if it really is undefined. If set incorrectly it will return the .min value as per the info.
This will mean that setting enabled/disabled only will grey out the control correctly - but not move the slider value for example.

@dceejay dceejay reopened this Apr 30, 2017
@dceejay dceejay moved this from Released on npm - 2.3.9 to Looking at - maybe done - on master - not on npm in Dashboard - Things to do Apr 30, 2017
@JaronrH
Copy link
Author

JaronrH commented Apr 30, 2017

This may be a little too kludgy but perhaps using a flag to trigger a content update vs state update would help? Ex: If updateStateOnly is present and True, only update the other display-based properties (color, enabled/disabled, etc.) and do not update the content (or pass on the message).

@dceejay
Copy link
Member

dceejay commented May 3, 2017

better fix now pushed to master as part of 2.3.10-beta

@dceejay dceejay added the bug label May 4, 2017
@dceejay dceejay self-assigned this May 4, 2017
@dceejay
Copy link
Member

dceejay commented May 5, 2017

re closed by fed10a5

@dceejay dceejay closed this as completed May 5, 2017
@dceejay dceejay removed this from Released on npm - 2.3.10 in Dashboard - Things to do Jul 7, 2017
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

2 participants