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

Fix Ember version detection #138

Merged

Conversation

@tancnle
Copy link
Contributor

commented Apr 19, 2019

Description

Fix Ember version detection.

Tested on https://emberjs.com/

@@ -963,7 +963,7 @@ var d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests = {
npm: 'ember-source',
test: function(win) {
var ember = win.Ember || win.Em;
if (ember && ember.propertyDidChange) {
if (ember) {

This comment has been minimized.

Copy link
@johnmichel

johnmichel Jun 29, 2019

Owner

Is there a secondary check that we can include with && to ensure that we’re not falsely detecting something else named Ember or Em on the window?

This comment has been minimized.

Copy link
@tancnle

tancnle Jul 2, 2019

Author Contributor

@johnmichel I have updated the PR to add a secondary check

@tancnle tancnle force-pushed the tancnle:tancnle/fix-ember-detection branch from f49aefd to 68d8766 Jul 2, 2019

@housseindjirdeh

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Thanks for this @tancnle! And apologies for not getting to this much sooner.

I don't know too much about ember, but I assume global variables wouldn't persist for many sites bundled with webpack/parcel/rollup? Do you think it would be better to look for DOM properties instead of globals like we did for React? (can be something we work on later, doesn't have to block this PR)

@housseindjirdeh housseindjirdeh merged commit e80654f into johnmichel:master Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.