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

breaking (UserAgent): Drop CDVUserAgentUtil and Implement for WKWebView #801

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

erisu
Copy link
Member

@erisu erisu commented Feb 28, 2020

Motivation and Context

  • Drop CDVUserAgentUtil which handled managing the User Agent string for UIWebView.
  • Add alternative implementation for WKWebView.

Description

  • Remove all existing UserAgent code that uses or associated with CDVUserAgentUtil.
  • Add to the WKWebView Engine configuration setup to set the user agent string.

Testing

  • npm t
  • cordova platform add
  • cordova build
  • Run in Simulator
  • Tested below test cases in config.xml

Test Cases

  • AppendUserAgent
  • AppendUserAgent w/ OverrideUserAgent
  • OverrideUserAgent

Checklist

  • I've run the tests to see all new and existing tests pass

@erisu erisu added this to the 6.0.0 milestone Feb 28, 2020
@codecov-io
Copy link

codecov-io commented Feb 28, 2020

Codecov Report

Merging #801 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #801   +/-   ##
======================================
  Coverage    74.2%   74.2%           
======================================
  Files          13      13           
  Lines        1849    1849           
======================================
  Hits         1372    1372           
  Misses        477     477

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2922437...703fbdc. Read the comment docs.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

You can override the whole user agent if using wkWebView.customUserAgent = userAgent;

@erisu erisu force-pushed the breaking/replace-cdvuseragentutil branch from 4cef3c9 to 1e62b8f Compare February 29, 2020 02:29
@erisu erisu requested a review from jcesarmobile February 29, 2020 02:29
@erisu
Copy link
Member Author

erisu commented Feb 29, 2020

@jcesarmobile I re-added back the wkWebView.customUserAgent for the OverrideUserAgent preference.

  • left the comment for OverrideUserAgent with the other configurations so they are gathered in one area.
  • updated the condition in the configuration setup method to still skip the append preference when override is set.
  • add the OverrideUserAgent condition in the pluginInitialize method where the wkWebView instance is created and availabe.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

I've made a comment about something I think should not be removed.

Other than that, the changes look good to me.

@erisu erisu requested a review from jcesarmobile March 2, 2020 01:08
@erisu erisu force-pushed the breaking/replace-cdvuseragentutil branch from 4eb3e07 to 703fbdc Compare March 2, 2020 01:11
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

LGTM

@erisu erisu merged commit 20248c1 into apache:master Mar 9, 2020
@erisu erisu deleted the breaking/replace-cdvuseragentutil branch March 9, 2020 13:23
@CVeniamin
Copy link

Went ahead and checked which plugins might be affected by this change.
I found that both cordova-plugin-file-transfer and cordova-plugin-wkwebview-engine depend on the previous API.

Although cordova-plugin-wkwebview-engine should now be "deprecated" and the change should not affect it as much since the WKWebview is the default engine.

@jcesarmobile
Copy link
Member

cordova-plugin-file-transfer is deprecated too

about cordova-plugin-wkwebview-engine, we were going to recommend using it for those who want to use file urls instead of the custom schemes, so maybe should be fixed to work on cordova-ios 6

@CVeniamin
Copy link

@jcesarmobile, thanks for the feedback!
I know that there are some use cases of cordova-plugin-file-transfer being used to play tracks offline combined with cordova-plugin-media for music players, that is why I have mentioned it.

@tryhardest
Copy link

Hopefully we can still use these plugins and upgrade specifically file transfer plugin. Sure would love to see anyone with authority release a new version of Ionic WKwebview plugin.

@vitto32
Copy link

vitto32 commented May 8, 2020

about cordova-plugin-wkwebview-engine, we were going to recommend using it for those who want to use file urls instead of the custom schemes, so maybe should be fixed to work on cordova-ios 6

I'm in that situation. #783 and this one are making things complicated.

I've spent a lot of time in the past replacing cordova-plugin-file-transfer, and I came up to a solution using cordova-plugin-wkwebview-file-xhr.

Now I'm trying to remove wkwebview-engine and wkwebview-file-xhr but I'm facing CORS issues when trying (in developement mode) to access file:// or app:// locations.

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.

6 participants