Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Use non-persistent session for requests by default #12694

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Jan 17, 2018

fix #12469

Test Plan:

  1. Turn on Brave payments in a fresh profile. Everything should work.
  2. Click 'check for updates'. It should not show console errors. If in a packaged build, it should say 'no updates available'.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@diracdeltas diracdeltas added this to the 0.21.x (Developer Channel) milestone Jan 17, 2018
@diracdeltas diracdeltas self-assigned this Jan 17, 2018
@codecov-io
Copy link

codecov-io commented Jan 17, 2018

Codecov Report

Merging #12694 into master will decrease coverage by <.01%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master   #12694      +/-   ##
==========================================
- Coverage   56.09%   56.09%   -0.01%     
==========================================
  Files         279      279              
  Lines       27302    27307       +5     
  Branches     4441     4442       +1     
==========================================
+ Hits        15316    15318       +2     
- Misses      11986    11989       +3
Flag Coverage Δ
#unittest 56.09% <28.57%> (-0.01%) ⬇️
Impacted Files Coverage Δ
js/lib/request.js 17.64% <28.57%> (+2.42%) ⬆️

bridiver
bridiver previously approved these changes Jan 22, 2018
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

maybe let defaultSession = getDefaultSession() would be better than setDefaultSession method with side effects?

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Per our convo, could it be updated to be more of a getter? ex:

getDefaultSession = function () {
  if (!defaultSession){
    defaultSession = session.fromPartition('default')
  }
  return defaultSession
}
const session = getDefaultSession()

Test Plan:
1. Turn on Brave payments in a fresh profile. Everything should work.
2. Click 'check for updates'. It should not show console errors. If in a packaged build, it should say 'no updates available'.
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

++

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look great! 😄

@diracdeltas diracdeltas merged commit 735b4ef into master Jan 22, 2018
@diracdeltas diracdeltas deleted the fix/12469 branch January 22, 2018 20:36
diracdeltas added a commit that referenced this pull request Jan 22, 2018
Use non-persistent session for requests by default
@diracdeltas
Copy link
Member Author

0.21.x: 375cd9f
master: 735b4ef

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

js/lib/request.js should use non-persistent session by default
4 participants