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(core): do not add window.cordova on web apps #4214

Merged
merged 4 commits into from
Feb 17, 2021

Conversation

elylucas
Copy link
Contributor

I wrapped the logic in the legacy handlers to only add the empty Cordova object and the native event handlers to only be executed when it is running on a device. This fixes the issue with Ionic Core wrongly identifying web apps as being identified as Cordova and hybrid apps as brought up in issue #4198.

@lincolnthree
Copy link

lincolnthree commented Feb 17, 2021

Glad I'm not crazy. I thought I was losing my mind for a while, trying to track down what I'd done that had caused this to start happening. Thanks for the fix!

@imhoffd imhoffd changed the title Cordova platform check fix fix(core): do not add window.cordova on web apps Feb 17, 2021
@imhoffd imhoffd merged commit 6d673ef into main Feb 17, 2021
@imhoffd imhoffd deleted the cordova-platform-check-fix branch February 17, 2021 17:41
@imhoffd
Copy link
Contributor

imhoffd commented Feb 17, 2021

@lincolnthree Thank you for reporting it!

@lincolnthree
Copy link

Hey @dwieeb, did this make it into the beta.3 build? Tried it out and it looks like the compiled sources do not include the fix:

image

sharktop:topdecked-unified lincoln$ npx cap doctor
💊   Capacitor Doctor  💊 

Latest Dependencies:

  @capacitor/cli: 2.4.6
  @capacitor/core: 2.4.6
  @capacitor/android: 2.4.6
  @capacitor/ios: 2.4.6

Installed Dependencies:

  @capacitor/cli: 3.0.0-beta.3
  @capacitor/core: 3.0.0-beta.3
  @capacitor/android: 3.0.0-beta.3
  @capacitor/ios: 3.0.0-beta.3

[success] iOS looking great! 👌
[success] Android looking great! 👌
sharktop:topdecked-unified lincoln$ 

@lincolnthree
Copy link

Actually hang on. I think there's some weird caching going on here. I see the updated dist/ in node_modules, but the ionic serve code does not seem to reflect the change, even with cache disabled in browser.

@lincolnthree
Copy link

Okay, confirmed! This is working :) Was a weird NPM multi-version precedence thing... Thanks again!

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