Routine maintenance 2023-07 #111
Conversation
docs.md
Outdated
| @@ -301,7 +301,7 @@ getAuthenticationContext() // undefined | |||
| * | |||
| ``` | |||
|
|
|||
| Returns **[String][32]** | |||
| Returns **[String][32]**  | |||
There was a problem hiding this comment.
This is weird, I need to look into it
There was a problem hiding this comment.
Later version(s?) of documentation expect an explanation for the return value. We were defining that in the description of both of these, instead of here.
| @@ -14,16 +14,16 @@ test('getAuthenticationContext returns undefined when lp_auth cookie is set to a | |||
| }) | |||
|
|
|||
| test('getAuthenticationContext returns undefined when lp_auth cookie does not contain a token', () => { | |||
| Cookies.set('lp_auth', {}) | |||
| Cookies.set('lp_auth', JSON.stringify({})) | |||
There was a problem hiding this comment.
In v3, Cookies.set no longer implicitly calls stringify. We aren't using this method in the code and we weren't using toJson() either, so it doesn't affect any source code.
There was a problem hiding this comment.
Sounds good. Any value in creating a testing util function/interface to Cookies that handles the JSON.stringify. There are 3 examples here 😏 (I'm good on holding off too but figured I would at least mention it)
There was a problem hiding this comment.
I could be convinced if the source code was using it, but I'm pretty ok with being repetitive in tests!
| const spyDocument = jest.spyOn(global, 'document', 'get') | ||
| spyDocument.mockImplementation(() => undefined) |
There was a problem hiding this comment.
This gets us test coverage for line 11 of getTokenFromDocument
| @@ -286,6 +305,16 @@ test('http sets basic auth header if `auth` is present', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| // Note: this isn't recommended and is super insecure, but technically the HTTP spec does allow it | |||
| test('http sets basic auth, even if username and password are not provided', () => { | |||
There was a problem hiding this comment.
This is a test for behavior that already exists. It gives us test coverage for the right-hand expression of auth.username || "" and auth.password || ""
| expect(isAuthenticated({ foo: 'bar' })).toEqual(true) | ||
| }) | ||
|
|
||
| test('isAuthenticated returns false when lp_auth cookie is empty', () => { |
There was a problem hiding this comment.
Gets us test coverage for line 55 of is-authenticated
| @@ -14,16 +14,16 @@ test('getAuthenticationContext returns undefined when lp_auth cookie is set to a | |||
| }) | |||
|
|
|||
| test('getAuthenticationContext returns undefined when lp_auth cookie does not contain a token', () => { | |||
| Cookies.set('lp_auth', {}) | |||
| Cookies.set('lp_auth', JSON.stringify({})) | |||
There was a problem hiding this comment.
Sounds good. Any value in creating a testing util function/interface to Cookies that handles the JSON.stringify. There are 3 examples here 😏 (I'm good on holding off too but figured I would at least mention it)
Items addressed
joinfromurl-joinin theadd-root-to-urlmiddleware. Ifrootwasn't defined, then the exception wouldn't be thrown but then the user would see "Uncaught TypeError: Failed to execute 'fetch' on 'Window': 1 argument required, but only 0 present." So either way, errors would be thrown (making this a backwards compatible change)Previous:
Current:
Out of Scope
yarn outdated. They both switched to ESM only packages, which by itself is not a huge issue but causes a real headache with eslint and jest, so we're not ready to accommodate that switch yet. I spiked on it for a few hours and didn't make much progress – an unfortunate reality of the schism in the community.