search

cozy / cozy-client-js

Javascript library to write Cozy applications
https://docs.cozy.io/en/cozy-client-js/README/
MIT License
11 stars 12 forks source link

fix: Use global fetch instead of closure #296

Closed taratatach closed 5 years ago

taratatach commented 5 years ago

Cozy Desktop uses cozy-client-js to interact with the user's Cozy and needs to be able to operate behind a proxy as well as define a custom User-Agent per user so support can follow their requests on the server. Electron offers better support for proxy configuration than Node so Cozy Desktop uses electron-fetch instead of isomorphic-fetch and electron-proxy-agent to customize the request's Agent with the proxy configuration. This means that Cozy Desktop needs to be able to replace the fetch method called by cozy-client-js as well so all requests made to the server use this custom configuration. The webpack's providePlugin plugin creates a closure with the given module around all modules calling the specified method. In our case, every module of cozy-client-js calling fetch will be wrapped in a closure binding fetch to the isomorphic-fetch imported by cozy-client-js. This renders overwritting it completely impossible for Cozy Desktop and thus prevents Cozy Desktop from upgrading cozy-client-js past version 0.15.0 since the change was introduced in version 0.15.1.

The proposed solution here is to import isomorphic-fetch which defines the global fetch variable in every cozy-client-js module who needs it, like the offline module which doesn't make a direct use of fetch but imports pouchdb-browser who needs fetch to be defined by the importing module or projet.

ptbrowne commented 5 years ago

I was trying to come up with alternative solutions as I do not like

but since isomorphic-fetch was already imported by webpack, the polyfill already happens without this change and isomorphic-fetch is already in the bundle. The only change then is that the import is more explicit, which is a good thing. A comment explaning why isomorphic-fetch is imported would be even better.

An alternative solution would be to require the user of the library to have the polyfill. More explicit but more burden on the user. It is what pouchdb-browser does so I think we should have gone this way in cozy-client-js. This is what we do now in cozy-client. This would be a breaking change and I do not think that we want to invest more in cozy-client-js at this point.

LGTM to me.

taratatach commented 5 years ago

I added a comment to explain why we import isomorphic-fetch in offline.js.