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

replace FileTransfer plugin with HttpClient #125

Merged
merged 1 commit into from
Jan 17, 2018
Merged

replace FileTransfer plugin with HttpClient #125

merged 1 commit into from
Jan 17, 2018

Conversation

westphalen
Copy link
Contributor

cordova-plugin-file-transfer deprecation #124
Tested, working, with the most recent Ionic version on Android. This should be backwards compatible with the existing @angular/common dependency, but I don't know the meaning of trustAllHosts, and if it needs to be implemented again.

@ihadeed
Copy link
Member

ihadeed commented Jan 13, 2018

Interesting.. thanks for the PR.

  • Setting trustAllHosts to true would tell the HTTP client to not verify SSL hosts. That would be inseucre, but helps in development. I think it should be fine to remove the option. SSLs are easy & free to get now, if someone is having problems with their self-signed SSL, they can easily get a trusted one in matter of minutes.
  • Did you compare the performance of FileTransfer vs HttpClient?

@westphalen
Copy link
Contributor Author

I haven't had the opportunity to compare performance, as my current project was completely unable to build with the file-transfer dependency. But it's a good point, I might find find time to set up a project with older dependencies, so I can test file-transfer vs HttpClient.

@westphalen
Copy link
Contributor Author

Performance tests weren't impressive, and a breaking change will be that image resources must have CORS headers to be loaded through xhr.

You can see my comparison here: https://github.com/westphalen/ionic-image-loader-benchmark

I don't know how we could improve performance. But something has to be done, since it is simply not possible to build a new Ionic project with ionic-image-loader at the moment.

@ihadeed
Copy link
Member

ihadeed commented Jan 15, 2018

  1. Why are you unable to build your project with FileTransfer dependency?
  2. I have a WIP plugin here that performs faster than FileTransfer and XHR: https://github.com/zyra/cordova-plugin-native-http ... I can probably finish it up and use it here.

@westphalen
Copy link
Contributor Author

To answer the dependency problem:

$ ionic start ionic-image-loader-test blank
? Would you like to integrate your new app with Cordova to target native iOS and Android? Yes
$ ionic cordova plugin add cordova-plugin-file
$ ionic cordova plugin add cordova-plugin-file-transfer
$ ionic cordova platform add android
Installing "cordova-plugin-file-transfer" for android
Failed to install 'cordova-plugin-file-transfer': CordovaError: Version of installed plugin: "cordova-plugin-file@6.0.1" does not satisfy dependency plugin requirement "cordova-plugin-file@^5.0.0". Try --force to use installed plugin as dependency.
    at /usr/local/lib/node_modules/cordova/node_modules/cordova-lib/src/plugman/install.js:557:37
    at _fulfilled (/usr/local/lib/node_modules/cordova/node_modules/q/q.js:787:54)
    at self.promiseDispatch.done (/usr/local/lib/node_modules/cordova/node_modules/q/q.js:816:30)
    at Promise.promise.promiseDispatch (/usr/local/lib/node_modules/cordova/node_modules/q/q.js:749:13)
    at /usr/local/lib/node_modules/cordova/node_modules/q/q.js:509:49
    at flush (/usr/local/lib/node_modules/cordova/node_modules/q/q.js:108:17)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)

Error: Version of installed plugin: "cordova-plugin-file@6.0.1" does not satisfy dependency plugin requirement "cordova-plugin-file@^5.0.0". Try --force to use installed plugin as dependency.

I don't know if you can inject the --force param somehow (ionic cordova platform add android -- --force doesn't work), but a temporary workaround might be to force a lower cordova-plugin-file version, but then you have to adjust versions of other packages that depend on the file plugin as well, and in some of my projects those are more than a few, so not very practical.

@ihadeed
Copy link
Member

ihadeed commented Jan 16, 2018

You can try two things.

Option A: use --force option (not sure if it will work)

cordova plugin add cordova-plugin-file-transfer --force

Option B: Downgrade the File plugin

cordova plugin rm cordova-plugin-file
cordova plugin add cordova-plugin-file@^5.0.0

You might need to delete plugins, platforms, node_modules, package-lock.json before doing that.. to make sure no old files interfere with anything.

@westphalen
Copy link
Contributor Author

Like I said, I have other dependencies that will start complaining if I downgrade cordova-plugin-file back to ^5.0.0, so we certainly need a long-term solution, unless you want to put instructions in README on using outdated dependencies.

@allanpoppe
Copy link

allanpoppe commented Jan 16, 2018

I agree with @westphalen. Cordova itself is recommending to use HttpClient over FileTransfer. I can't have File plugin outdated neither. I'm testing his fork and it's playing nicely on Android.
This is a nice module @ihadeed. Let us keep using it.

@ihadeed
Copy link
Member

ihadeed commented Jan 16, 2018

@allanpoppe Yeah I'm thinking to just merge this and move forward in this direction. The purpose of this plugin is to (1) load images faster than alternative methods and (2) cache them. This will still keep the project on track.

If I ever finish that native HTTP project, I'll implement in this plugin, and maybe make it optional (give users option between Native or HttpClient).

@allanpoppe
Copy link

Very nice @ihadeed !

@allanpoppe
Copy link

Any news on accepting this PR this @ihadeed ?

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