arantes555 / electron-fetch

A light-weight module that brings window.fetch to the background process of Electron
Other
133 stars 21 forks source link

Use Electron's session cookies #26

Closed taratatach closed 4 years ago

taratatach commented 4 years ago

Fixes #21

In order for net requests to match a fetch request cookie behavior, an option can now be passed to use the session cookie store (see https://github.com/electron/electron/pull/22704).

To keep the default interfaces as close as possible between the Node and the Electron usages, we add a new Request useSessionCookies option to tell electron-fetch whether it should use the stored Electron's session cookies or not.

This option matches the Electron net useSessionCookies option.

By default, the option will be disabled.

codecov[bot] commented 4 years ago

Codecov Report

Merging #26 into master will decrease coverage by 0.23%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   99.51%   99.28%   -0.24%     
==========================================
  Files           6        6              
  Lines         415      420       +5     
  Branches      135      138       +3     
==========================================
+ Hits          413      417       +4     
  Misses          1        1              
- Partials        1        2       +1     
Impacted Files Coverage Δ
src/index.js 96.42% <100.00%> (-1.17%) :arrow_down:
src/request.js 100.00% <100.00%> (ø)

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 ef14bd8...d97f4b9. Read the comment docs.

arantes555 commented 4 years ago

electron-fetch should always pass this option when making requests with net since the goal is to mimic the Fetch API.

I am not sure about that...

I want to keep electron-fetch functioning as close as possible when running on electron and on node. I think the best would be to add an option, in order to enable this behaviour.

taratatach commented 4 years ago

I think the best would be to add an option, in order to enable this behaviour.

I'm fine with that but doesn't it mean the behavior on Node does not match the behavior of the original Fetch API?

taratatach commented 4 years ago

@arantes555 I added a commit adding a useSessionCookies option as requested.

taratatach commented 4 years ago

@arantes555 Have you had the time to give a look to the latest commit?

arantes555 commented 4 years ago

@taratatach Merged !

I had to skip your new tests on electron < 7, and remove your strict check of the cookies stored in the session to make it work on electron@10 that was just released yesterday (they added attributes to the cookies, so your deepEqual failed).

Also, if you post other PRs in the future, do not hesitate to edit the README.md, and the index.d.ts typings ! (it's totally OK to let me do it if you are not used to dealing with typings)

Again, thank you very much ! I'm releasing it right now as 1.6.0

taratatach commented 4 years ago

@arantes555 Awesome! Thank you. I was simply not aware of the typings file and did not think of the Readme. I'll keep it in mind for next time.