Crash on incorrect user / password with kinto account

[leplatrem]

Steps to reproduce:

• npm start
• Use kinto demo server https://kinto.dev.mozaws.net/v1/
• Pick kinto account
• Use admin and a random input for password

The app crashes with:

11:41:26.919 TypeError: errorData is undefined
    getErrorDetails notifications.ts:17
    notifyError notifications.ts:100
    setupSession$ session.ts:133
    tryCatch runtime.js:63
    invoke runtime.js:293
    defineIteratorMethods runtime.js:118
    Redux 42
    AuthForm AuthForm.tsx:469
    Form Form.js:220
    React 8
    unstable_runWithPriority scheduler.development.js:653
    React 5
    unstable_runWithPriority scheduler.development.js:653
    React 17
    unstable_runWithPriority scheduler.development.js:653
    React 6
notifications.ts:97:10

11:41:26.924 The above error occurred in task setupSession
    created by takeEvery(SESSION_SETUP, setupSession)
    created by rootSaga
Tasks cancelled due to error:
takeEvery(SESSION_SETUP, setupSession)
takeEvery(SESSION_SERVER_CHANGE, serverChange)
takeEvery(SESSION_GET_SERVERINFO, getServerInfo)
takeEvery(SESSION_BUCKETS_REQUEST, listBuckets)
takeEvery(SESSION_LOGOUT, sessionLogout)
takeEvery(SESSION_COPY_AUTHENTICATION_HEADER, sessionCopyAuthenticationHeader)
takeEvery(ROUTE_REDIRECT, routeRedirect)
takeEvery(ROUTE_UPDATED, routeUpdated)
takeEvery(BUCKET_CREATE_REQUEST, createBucket)
takeEvery(BUCKET_UPDATE_REQUEST, updateBucket)
takeEvery(BUCKET_DELETE_REQUEST, deleteBucket)
takeEvery(BUCKET_COLLECTIONS_REQUEST, listBucketCollections)
takeEvery(BUCKET_COLLECTIONS_NEXT_REQUEST, listBucketNextCollections)
takeEvery(BUCKET_HISTORY_REQUEST, listHistory)
takeEvery(BUCKET_HISTORY_NEXT_REQUEST, listNextHistory)
takeEvery(COLLECTION_CREATE_REQUEST, createCollection)
takeEvery(COLLECTION_UPDATE_REQUEST, updateCollection)
takeEvery(COLLECTION_DELETE_REQUEST, deleteCollection)
takeEvery(GROUP_CREATE_REQUEST, createGroup)
takeEvery(GROUP_UPDATE_REQUEST, updateGroup)
takeEvery(GROUP_DELETE_REQUEST, deleteGroup)
takeEvery(GROUP_HISTORY_REQUEST, listHistory)
takeEvery(GROUP_HISTORY_NEXT_REQUEST, listNextHistory)
takeEvery(COLLECTION_RECORDS_REQUEST, listRecords)
takeEvery(COLLECTION_RECORDS_NEXT_REQUEST, listNextRecords)
takeEvery(COLLECTION_HISTORY_REQUEST, listHistory)
takeEvery(COLLECTION_HISTORY_NEXT_REQUEST, listNextHistory)
takeEvery(RECORD_CREATE_REQUEST, createRecord)
takeEvery(RECORD_BULK_CREATE_REQUEST, bulkCreateRecords)
takeEvery(RECORD_UPDATE_REQUEST, updateRecord)
takeEvery(RECORD_DELETE_REQUEST, deleteRecord)
takeEvery(RECORD_HISTORY_REQUEST, listHistory)
takeEvery(RECORD_HISTORY_NEXT_REQUEST, listNextHistory)
takeEvery(ATTACHMENT_DELETE_REQUEST, deleteAttachment)
takeEvery(COLLECTION_RECORDS_REQUEST, onCollectionRecordsRequest)
takeEvery(PLUGIN_REVIEW_REQUEST, handleRequestReview)
takeEvery(PLUGIN_ROLLBACK_CHANGES, handleRollbackChanges)
takeEvery(PLUGIN_DECLINE_REQUEST, handleDeclineChanges)
takeEvery(PLUGIN_SIGNOFF_REQUEST, handleApproveChanges) io-6de156f3.js:112

mozilla/remote-settings#128

[grahamalama]

Opened a PR (#2131) that I think will fix this bug, but a few comments here for discussion in the meantime:

• This bug would have been caught earlier had strict mode been enabled. It might worth flipping that on to prevent these kinds of bugs, but there will be some added typing overhead as a result. It will also be a bit of a chore. tsc reports 321 errors with strict mode enabled.
• Another thing that would have possibly caught this bug earlier would have been to not mock notifyError in the saga tests (as I wrote in 2131's description). I don't know if we want to remove all notifyError mocks, but I generally tend to favor not mocking when possible. Either that, or explicitly writing tests for those notification action creators, though it seems Redux docs tend to steer people away from that approach these days.
• On the subject of saga testing, we may want to consider using Redux Saga Test Plan instead of manually .next()ing through the generators.

[leplatrem]

Thanks for looking into this.

I think each of the above points could be put into issues!

Switching to strict makes sense, but yeah since this project was migrated to Typescript years after its creation, this could be tedious.

Same for redux, we used this pattern in this project when the ecosystem was really young. It would be great if we could align it with the latest recommandations.

[grahamalama]

It would be great if we could align it with the latest recommendations.

Agreed, but that'll also be a bit of a chore. Redux seems to encourage their new(ish) Redux Toolkit these days, which is pretty different than the "old way" of doing Redux in React. One nice thing though is that we could slowly migrate slices of the store to use it since it can be made backwards compatible with the old way. I could try migrating one slice of the store to see how painful the migration would be.