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

Update buildMouseEvent to use MouseEvent constructor if it is present #387

Merged
merged 3 commits into from
Jul 31, 2018
Merged

Update buildMouseEvent to use MouseEvent constructor if it is present #387

merged 3 commits into from
Jul 31, 2018

Conversation

ggayowsky
Copy link
Contributor

Closes #266 by using the MouseEvent constructor in buildMouseEvent if it is present. Otherwise, it falls back to the existing initMouseEvent function for compatibility with IE 9/10

} catch (e) {
// left intentionally blank
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, aren’t some errors valid here? E.g. when MouseEvent is present but you pass invalid options or something?

Copy link
Member

Choose a reason for hiding this comment

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

maybe we could make a constant in module scope to track if `MouseEvent‘ is functional, then remove the try/catch inline?

Copy link
Contributor Author

@ggayowsky ggayowsky Jun 19, 2018

Choose a reason for hiding this comment

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

Was following the pattern from buildKeyboardEvent, but I agree that the solution where checking if MouseEvent exists is better

@@ -1,5 +1,15 @@
import { merge } from '@ember/polyfills';

// eslint-disable-next-line require-jsdoc
function checkMouseEventExistence() {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you memoize this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an example of where else Memoization it is done in this repo? Or another example I can emulate?

Copy link
Member

Choose a reason for hiding this comment

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

Something like:

const MOUSE_EVENT_CONSTRUCTOR = (() => {
  try {
    new MouseEvent(‘test’);
    return true;
  } catch(e) {
    return false;
  }
})();

@rwjblue rwjblue requested a review from cibernox June 20, 2018 01:59
@ggayowsky
Copy link
Contributor Author

Bump?

@cibernox
Copy link
Contributor

It looks good to me. /cc @rwjblue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants