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

Check observer existence in domobserver methods #60

Closed
wants to merge 3 commits into from

Conversation

ranmocy
Copy link
Contributor

@ranmocy ranmocy commented Sep 24, 2019

No description provided.

@marijnh
Copy link
Member

marijnh commented Sep 24, 2019

I think flush has the same problem, and I'm wondering if it doesn't make more sense to remove all of these checks, given that ProseMirror isn't going to work anyway, on platforms that don't have mutationobserver. Did you run into this in a real situation? If so, which browser was that?

@ranmocy
Copy link
Contributor Author

ranmocy commented Sep 25, 2019

Updated for flush().

Not in production, but in tests with jsdom framework. It's undefined function issue since this.observer.takeRecords() is called but this.observer is undefined.

jsdom added the support of mutationobserver in jsdom/jsdom#639, and I'm waiting for next version of jest to include the version bump.

Even though it will eventually work, I still think it's better to do so for correctness. The observer should either be nullable in the caller, or silently no-op when methods are called.

@ranmocy ranmocy changed the title Check observer existence in domobserver.stop() Check observer existence in domobserver methods Sep 25, 2019
src/domobserver.js Outdated Show resolved Hide resolved
@marijnh
Copy link
Member

marijnh commented Sep 25, 2019

Thank you! Merged as f329f82

@marijnh marijnh closed this Sep 25, 2019
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.

2 participants