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(ios): re-implement user agent overwrite #268

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Conversation

timbru31
Copy link
Member

@timbru31 timbru31 commented Aug 25, 2020

This closes #263
This closes #258

Platforms affected

iOS

Motivation and Context

This solves the issue in #258 and makes the plugin compatible with cordova-ios@6.
It also re-implements the user agent overwrite.

Description

Sadly, there is no "nice" way to obtain the WKWebView user agent other than evaluating JavaScript.

Testing

My local fork compiles and has the correct user agent attached when making the network requests.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

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.

looks good

@NiklasMerz NiklasMerz removed their request for review August 25, 2020 18:02
@timbru31 timbru31 merged commit 64bfd15 into master Aug 25, 2020
@timbru31 timbru31 deleted the fix/user-agent branch August 25, 2020 21:44
@sithwarrior
Copy link

Great stuff, any chance of getting this pushed to npm, so we can start using it?

@timbru31
Copy link
Member Author

timbru31 commented Aug 27, 2020 via email

@jpike88
Copy link

jpike88 commented Sep 8, 2020

+1 to a fast release, as this is blocking builds.

@christiaan
Copy link
Contributor

christiaan commented Sep 22, 2020

@timbru31 this appears to break adding all the configured headers for me.

My assumption is that this call does not block untill the completionHandler is executed.

[self.webViewEngine evaluateJavaScript:@"navigator.userAgent" completionHandler:^(NSString* userAgent, NSError* error) {

I looked at https://developer.apple.com/documentation/webkit/wkwebview/1415017-evaluatejavascript but it does not indicate tell if it blocks till the completionHandler has run.

EDIT: I've added some NSLog statements and indeed it does not block when calling evaluateJavascript.

@timbru31
Copy link
Member Author

Thanks for investigating, I'll take a look.

@christiaan
Copy link
Contributor

That is a quick reply :)

I was considering moving the userAgent determination to the plugin is initialization.
This would also reduce the amount of evaluateJavascript calls to just one instead of for each request.

@timbru31
Copy link
Member Author

Are you willing to prepare a PR for that? 👍

@christiaan
Copy link
Contributor

Seems that is also not possible. Since pluginInitialize is synchronous the same problem can occur.

Since the headers can be set from javascript, including the User-Agent header. What about adding a default in javascript instead and removing the User-Agent from all the native code?

@christiaan
Copy link
Contributor

I've looked at the Android and Windows implementation, both do not add the User-Agent header in the plugin code.

Am I missing something or can it just be removed?

@christiaan
Copy link
Contributor

I've created #284 for this issue.

MayankLogiciel added a commit to MayankLogiciel/cordova-plugin-file-transfer that referenced this pull request Oct 5, 2020
* Add or update GitHub pull request and issue template
* docs: undeprecate the plugin (apache#267)
Co-authored-by: エリス <erisu@users.noreply.github.com>
* fix(ios): re-implement user agent overwrite (apache#268)
This closes apache#263 apache#258
* chore(npm): adds ignore list (apache#269)
* fix!: remove deprecated platforms (apache#270)
* chore: bump version to 2.0.0-dev (apache#273)
* chore: package cleanup (apache#272)
* refactor(eslint): use cordova-eslint /w fix (apache#275)
* chore: adds package-lock file (apache#274)
* doc: Improve progressEvent documentation (apache#280)

Co-authored-by: Jan Piotrowski <piotrowski+git@gmail.com>
Co-authored-by: Tim Brust <github@timbrust.de>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: Norman Breau <norman@normanbreau.com>
petewalker added a commit to OurPeople/cordova-plugin-file-transfer that referenced this pull request Nov 9, 2020
fabiofabbri84 pushed a commit to cimatti/cordova-plugin-file-transfer that referenced this pull request Jul 17, 2023
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.

No known instance method for selector 'userAgent' in CDVFileTransfer.m
7 participants