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.

Sign up for GitHub

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

Jump to bottom

guard against null _disabledEventEmissions in Observer#fireEvent #1970

Merged
merged 1 commit into from Jun 23, 2020
Merged

guard against null _disabledEventEmissions in Observer#fireEvent #1970

merged 1 commit into from Jun 23, 2020

Conversation

osheroff
Copy link
Contributor

@osheroff osheroff commented Jun 16, 2020 •
edited by thijstriemstra

due to the odd way the RegionsPlugin pulls in the methods in util.Observer, it needs to setup a private array or it crashes in fireEvent.

I could also fix this in Observer by checking for the existence of the array. I dunno. both seem odd.

fixes #1975

@osheroff
Copy link
Contributor Author

hmmm... this doesn't seem to be fixing the crash

@osheroff
Copy link
Contributor Author

nevermind. I'm doing something dumb.

@osheroff osheroff closed this Jun 16, 2020
@coveralls
Copy link

coveralls commented Jun 16, 2020 •
edited

Coverage Status

Coverage increased (+0.01%) to 80.512% when pulling c5ac3f0 on osheroff/fix_region_plugin_fire into f8544de on master.

@osheroff osheroff reopened this Jun 23, 2020
@osheroff
Copy link
Contributor Author

@katspaugh I was doing something dumb in my own code and in this fix. But it does need to be fixed, regions are totally broken in master.

@osheroff osheroff mentioned this pull request Jun 23, 2020
Merged
1 task
@cadavre
Copy link
Contributor

cadavre commented Jun 23, 2020

The problem is with https://github.com/katspaugh/wavesurfer.js/blob/master/src/util/observer.js#L124

Inside fireEvent – this is being an argument passed when triggering the event, not Observer itself.

It looks like Observer is somehow broken, not Regions plugin.

@osheroff osheroff mentioned this pull request Jun 23, 2020
Merged
@osheroff
Copy link
Contributor Author

@cadavre test it yourself before and after this patch. this does indeed fix it...

@osheroff
Copy link
Contributor Author

@katspaugh updated with single quotes

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Jun 23, 2020 •
edited

It looks like Observer is somehow broken, not Regions plugin.

Agreed.

All of the plugins that use observer are broken currently, e.g. microphone plugin. I don't like adding this change to every plugin (unless there's really no other way).

@osheroff
Copy link
Contributor Author

well @thijstriemstra here's the other approach...

thijstriemstra
thijstriemstra suggested changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

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

I think this looks better. Can you add a changelog entry and update PR title?

@thijstriemstra thijstriemstra mentioned this pull request Jun 23, 2020
Closed
@osheroff osheroff changed the title fix for RegionPlugin#fireEvent guard against null _disabledEventEmissions in Observer#fireEvent Jun 23, 2020
@osheroff
guard against null _disabledEventEmissions in Observer#fireEvent
c5ac3f0 
the regions plugin notably was using all the functions of Observer
without calling the constructor, triggering a crash
@osheroff osheroff force-pushed the osheroff/fix_region_plugin_fire branch from 880f11d to c5ac3f0 Compare June 23, 2020 18:58
@osheroff
Copy link
Contributor Author

@thijstriemstra updated

thijstriemstra
thijstriemstra approved these changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

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

LGTM

@osheroff osheroff merged commit 4e2375e into master Jun 23, 2020
@thijstriemstra thijstriemstra deleted the osheroff/fix_region_plugin_fire branch June 23, 2020 21:06
sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
@osheroff @sandiz
guard against null _disabledEventEmissions in Observer#fireEvent (kat…
0410cfb 
…spaugh#1970)

the regions plugin notably was using all the functions of Observer
without calling the constructor, triggering a crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cadavre cadavre cadavre left review comments

@thijstriemstra thijstriemstra thijstriemstra approved these changes

@katspaugh katspaugh katspaugh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot read property 'includes' of undefined
5 participants
@osheroff @coveralls @cadavre @thijstriemstra @katspaugh