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(platform): compatibility with Android 4.4 #18387

Merged
merged 3 commits into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@cezarykluczynski
Copy link
Contributor

commented May 25, 2019

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

Currently calling Platform.is() method throws an exception on Android 4.4. That's because Array.prototype.includes is not supported in WebView up to Android 4.4.4, according to https://caniuse.com/#feat=array-includes

What is the new behavior?

Platform.isPlatform() can be called when building project for Android 4.4, because it will now use good old array.indexOf(x) > -1.

Does this introduce a breaking change?

  • Yes
  • No

Other information

No other information.

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Hi @cezarykluczynski,

Thanks for the PR! Stencil includes a polyfill for Array.includes for browsers that need it, so this change shouldn't be needed: https://github.com/ionic-team/stencil/blob/master/readme.md#polyfills

Are you able to describe how to reproduce the error you are describing? I am running ['a','b','c'].includes('a') on an Android 4.4 device and it appears to be working properly.

Thanks!

@cezarykluczynski

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@liamdebeasi The best way to reproduce would be to call the problematic function, platform.is(), from a project with the following dependencies:

{
  "dependencies": {
    "@angular/common": "7.2.2",
    "@angular/core": "7.2.2",
    "@angular/forms": "7.2.2",
    "@angular/http": "7.2.2",
    "@angular/platform-browser": "7.2.2",
    "@angular/platform-browser-dynamic": "7.2.2",
    "@angular/router": "7.2.2",
    "@ionic-native/core": "5.0.0",
    "@ionic-native/device": "5.4.0",
    "@ionic-native/file": "5.5.0",
    "@ionic-native/file-transfer": "5.4.0",
    "@ionic-native/ionic-webview": "5.5.0",
    "@ionic-native/local-notifications": "5.4.0",
    "@ionic-native/splash-screen": "5.0.0",
    "@ionic-native/status-bar": "5.0.0",
    "@ionic/angular": "4.2.0",
    "@ionic/storage": "2.2.0",
    "appium-chromedriver": "4.6.0",
    "cordova-android": "8.0.0",
    "cordova-plugin-badge": "0.8.8",
    "cordova-plugin-device": "2.0.2",
    "cordova-plugin-file": "6.0.1",
    "cordova-plugin-file-transfer": "1.7.1",
    "cordova-plugin-local-notification": "0.9.0-beta.2",
    "cordova-plugin-mediascanner": "0.1.3",
    "core-js": "2.5.4",
    "ionic-image-loader": "7.0.0-beta.2",
    "rxjs": "6.3.3",
    "sw-toolbox": "3.6.0",
    "ts-md5": "1.2.4",
    "zone.js": "0.9.1"
  },
  "devDependencies": {
    "@angular-devkit/architect": "0.12.3",
    "@angular-devkit/build-angular": "0.13.0",
    "@angular-devkit/core": "7.2.3",
    "@angular-devkit/schematics": "7.2.3",
    "@angular/cli": "7.3.1",
    "@angular/compiler": "7.2.2",
    "@angular/compiler-cli": "7.2.2",
    "@angular/language-service": "7.2.2",
    "@ionic/angular-toolkit": "1.4.0",
    "@types/jasmine": "2.8.8",
    "@types/jasminewd2": "2.0.3",
    "@types/node": "10.14.2",
    "codelyzer": "4.5.0",
    "cordova-plugin-ionic-keyboard": "2.0.5",
    "cordova-plugin-ionic-webview": "1.1.19",
    "cordova-plugin-splashscreen": "5.0.2",
    "cordova-plugin-statusbar": "2.4.2",
    "cordova-plugin-whitelist": "1.3.3",
    "jasmine-core": "2.99.1",
    "jasmine-spec-reporter": "4.2.1",
    "karma": "3.1.4",
    "karma-chrome-launcher": "2.2.0",
    "karma-coverage-istanbul-reporter": "2.0.1",
    "karma-jasmine": "1.1.2",
    "karma-jasmine-html-reporter": "0.2.2",
    "protractor": "5.4.0",
    "ts-node": "8.0.0",
    "tslint": "5.12.0",
    "typescript": "3.1.6",
    "wdio-appium-service": "0.2.3",
    "wdio-jasmine-framework": "0.3.6",
    "wdio-spec-reporter": "0.1.5",
    "webdriverio": "4.13.2",
    "ws": "3.3.2"
  }
}

I'm also not sure about the Stencil part. Array.includes seems to be a polyfill loaded "on-demand", but does that mean I have to manually supply the demand somehow?

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Ok thanks I'll see if I can reproduce. What device did you test this on?

In terms of the polyfills, "on-demand" means a polyfill is loaded automatically if needed. For a browser that does not support Array.includes, Stencil would detect that and automatically load the appropriate polyfill. The user should not have to manually supply anything.

@cezarykluczynski

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I'm testing on Android Studio-created emulator, Nexus S with Android 4.4, 480x800 screen size, and API version 19.

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I have tested the following code on that same simulator:

console.log(this.platform.is('android'), ['a','b','c'].includes('d'), ['a','b','c'].includes('a'));

Additionally, I have tested this on Ionic 4.2.0 (as per your package.json above) and Ionic 4.4.2. Everything seems to be working for me.

Can you provide a code reproduction of this issue happening? Thanks!

@cezarykluczynski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

@liamdebeasi Here's a reduced test case: https://github.com/cezarykluczynski/ionic-18387-reduced-test-case

npm run build-android-emulator willl build an apk that, once uploaded to a Android 4.4 emulator, will produce the error, if you inspect from chrome://inspect/#devices

With the fix I'm proposing, there would be no error.

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thanks for the follow up! It looks like this code is being run before the polyfills are fully loaded, which is why this issue is happening. Will merge shortly. Thanks!

@liamdebeasi liamdebeasi merged commit 54bdb36 into ionic-team:master Jun 10, 2019

1 check passed

build Workflow: build
Details
@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thank you! 🎉

@zhangbo66666

This comment has been minimized.

Copy link

commented Jun 10, 2019

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