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

Checking a couple vars for truthiness. #173

Closed
wants to merge 1 commit into from

Conversation

adityamenon
Copy link

I am using ImageMapster with angular. When the two variables
I have corrected inside the refreshSelections and clearHighlight
functions are not set, console errors are generated.

Exact situation is me calling:
$('#mapster').mapster('set', false, regionList.join());
where regionList.join() returns an empty string in one case. When
there are no previous selections, errors occur when the two vars
I corrected are attempted to be used while they are actually null.

I am guessing Mapster actually has safeguards against reaching this
code at a higher level when things are empty, but I'm using it only
as a simple highlight-storage area and tracking all state inside angular,
so probably that code is unable to kick in.

I am using ImageMapster with angular. When the two variables
I have corrected inside the refreshSelections and clearHighlight
functions are not set, console errors are generated.

Exact situation is me calling:
$('#mapster').mapster('set', false, regionList.join());
where regionList.join() returns an empty string in one case. When
there *are* no previous selections, errors occur when the two vars
I corrected are attempted to be used while they are actually null.

I am guessing Mapster actually has safeguards against reaching this
code at a higher level when things are empty, but I'm using it only
as a simple highlight-storage area and tracking all state inside angular,
so probably that code is unable to kick in.
@techfg
Copy link
Collaborator

techfg commented Jan 26, 2021

Hello @adityamenon -

Thank you for your contribution, apologies on the long delay on getting to you regarding this.

I've reviewed the changes you proposed and agree that if these variables are non-truthy, checking them for truthyness will avoid an error/exception regarding an undefined variable.

With that said, I've also reviewed the code regarding what situations would result in these variables not having a value and cannot seem to identify what would lead to this scenario assuming mapster was being used as intended. You mention you are using it in an "unconventional" manner so, as you suggest, I'm guessing that the way you were using it led to this condition. I've tried multiple methods of trying to re-create your scenario but unfortunately have been unable to do so. You didn't mention in your OP what version of the code you were using so I assumed that you were using the latest version available at the time of your submission (1.2.14-beta1).

I'm hesitant to add these conditional checks because in "expected" usage, these variables should be non-null and if they are null, then mapster is in an inconsistent/unexpected state and its better to fail with an exception than proceed and encounter further unintended consequences. Along this line, I did identify a bug in Mapster recently (#351) where it allows you to invoke methods on an uninitialized instance - possibly this was your scenario but I even tried this scenario and couldn't reproduce your scenario.

Long story short, would be happy to take a closer look at this one if you are able to provide a repro of the issue so that I can identify whether or not the variables were null/undefined because of unexpected usage pattern or if its an excepted usage pattern and an underlying issue with the code.

Closing this for now but if you happen to recall the scenario and even better have a repro, happy to re-open and evaluate. Thanks again!

@techfg techfg closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants