@uppy/companion-client: migrate to private properties (#3057)

* @uppy/companion-client: migrate to private properties

* remove duplicate i18nInit — it’s in BasePlugin

* Revert "remove duplicate i18nInit — it’s in BasePlugin"

This reverts commit 93fb08092632a050710df03294e59bc4ae077bb3.

Co-authored-by: Artur Paikin <artur@arturpaikin.com>

packages/@uppy/companion-client/src/RequestClient.js

@@ -8,9 +8,33 @@ function stripSlash (url) {
   return url.replace(/\/$/, '')
 }
 
+async function handleJSONResponse (res) {
+  if (res.status === 401) {
+    throw new AuthError()
+  }
+
+  const jsonPromise = res.json()
+
+  if (res.status < 200 || res.status > 300) {
+    let errMsg = `Failed request with status: ${res.status}. ${res.statusText}`
+    try {
+      const errData = await jsonPromise
+      errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg
+      errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg
+    } finally {
+      // eslint-disable-next-line no-unsafe-finally
+      throw new Error(errMsg)
+    }
+  }
+  return jsonPromise
+}
+
 module.exports = class RequestClient {
+  // eslint-disable-next-line global-require
   static VERSION = require('../package.json').version
 
+  #getPostResponseFunc = skip => response => (skip ? response : this.onReceiveResponse(response))
+
   constructor (uppy, opts) {
     this.uppy = uppy
     this.opts = opts
@@ -25,32 +49,20 @@ module.exports = class RequestClient {
     return stripSlash(companion && companion[host] ? companion[host] : host)
   }
 
-  get defaultHeaders () {
-    return {
-      Accept: 'application/json',
-      'Content-Type': 'application/json',
-      'Uppy-Versions': `@uppy/companion-client=${RequestClient.VERSION}`,
-    }
+  static defaultHeaders ={
+    Accept: 'application/json',
+    'Content-Type': 'application/json',
+    'Uppy-Versions': `@uppy/companion-client=${RequestClient.VERSION}`,
   }
 
   headers () {
     const userHeaders = this.opts.companionHeaders || {}
     return Promise.resolve({
-      ...this.defaultHeaders,
+      ...RequestClient.defaultHeaders,
       ...userHeaders,
     })
   }
 
-  _getPostResponseFunc (skip) {
-    return (response) => {
-      if (!skip) {
-        return this.onReceiveResponse(response)
-      }
-
-      return response
-    }
-  }
-
   onReceiveResponse (response) {
     const state = this.uppy.getState()
     const companion = state.companion || {}
@@ -65,28 +77,22 @@ module.exports = class RequestClient {
     return response
   }
 
-  _getUrl (url) {
+  #getUrl (url) {
     if (/^(https?:|)\/\//.test(url)) {
       return url
     }
     return `${this.hostname}/${url}`
   }
 
-  _json (res) {
-    if (res.status === 401) {
-      throw new AuthError()
-    }
-
-    if (res.status < 200 || res.status > 300) {
-      let errMsg = `Failed request with status: ${res.status}. ${res.statusText}`
-      return res.json()
-        .then((errData) => {
-          errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg
-          errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg
-          throw new Error(errMsg)
-        }).catch(() => { throw new Error(errMsg) })
+  #errorHandler (method, path) {
+    return (err) => {
+      if (!err?.isAuthError) {
+        const error = new Error(`Could not ${method} ${this.#getUrl(path)}`)
+        error.cause = err
+        err = error // eslint-disable-line no-param-reassign
+      }
+      return Promise.reject(err)
     }
-    return res.json()
   }
 
   preflight (path) {
@@ -94,7 +100,7 @@ module.exports = class RequestClient {
       return Promise.resolve(this.allowedHeaders.slice())
     }
 
-    return fetch(this._getUrl(path), {
+    return fetch(this.#getUrl(path), {
       method: 'OPTIONS',
     })
       .then((response) => {
@@ -117,9 +123,9 @@ module.exports = class RequestClient {
       .then(([allowedHeaders, headers]) => {
         // filter to keep only allowed Headers
         Object.keys(headers).forEach((header) => {
-          if (allowedHeaders.indexOf(header.toLowerCase()) === -1) {
-            this.uppy.log(`[CompanionClient] excluding unallowed header ${header}`)
-            delete headers[header]
+          if (!allowedHeaders.includes(header.toLowerCase())) {
+            this.uppy.log(`[CompanionClient] excluding disallowed header ${header}`)
+            delete headers[header] // eslint-disable-line no-param-reassign
           }
         })
 
@@ -128,55 +134,43 @@ module.exports = class RequestClient {
   }
 
   get (path, skipPostResponse) {
+    const method = 'get'
     return this.preflightAndHeaders(path)
-      .then((headers) => fetchWithNetworkError(this._getUrl(path), {
-        method: 'get',
+      .then((headers) => fetchWithNetworkError(this.#getUrl(path), {
+        method,
         headers,
         credentials: this.opts.companionCookiesRule || 'same-origin',
       }))
-      .then(this._getPostResponseFunc(skipPostResponse))
-      .then((res) => this._json(res))
-      .catch((err) => {
-        if (!err.isAuthError) {
-          err.message = `Could not get ${this._getUrl(path)}. ${err.message}`
-        }
-        return Promise.reject(err)
-      })
+      .then(this.#getPostResponseFunc(skipPostResponse))
+      .then(handleJSONResponse)
+      .catch(this.#errorHandler(method, path))
   }
 
   post (path, data, skipPostResponse) {
+    const method = 'post'
     return this.preflightAndHeaders(path)
-      .then((headers) => fetchWithNetworkError(this._getUrl(path), {
-        method: 'post',
+      .then((headers) => fetchWithNetworkError(this.#getUrl(path), {
+        method,
         headers,
         credentials: this.opts.companionCookiesRule || 'same-origin',
         body: JSON.stringify(data),
       }))
-      .then(this._getPostResponseFunc(skipPostResponse))
-      .then((res) => this._json(res))
-      .catch((err) => {
-        if (!err.isAuthError) {
-          err.message = `Could not post ${this._getUrl(path)}. ${err.message}`
-        }
-        return Promise.reject(err)
-      })
+      .then(this.#getPostResponseFunc(skipPostResponse))
+      .then(handleJSONResponse)
+      .catch(this.#errorHandler(method, path))
   }
 
   delete (path, data, skipPostResponse) {
+    const method = 'delete'
     return this.preflightAndHeaders(path)
       .then((headers) => fetchWithNetworkError(`${this.hostname}/${path}`, {
-        method: 'delete',
+        method,
         headers,
         credentials: this.opts.companionCookiesRule || 'same-origin',
         body: data ? JSON.stringify(data) : null,
       }))
-      .then(this._getPostResponseFunc(skipPostResponse))
-      .then((res) => this._json(res))
-      .catch((err) => {
-        if (!err.isAuthError) {
-          err.message = `Could not delete ${this._getUrl(path)}. ${err.message}`
-        }
-        return Promise.reject(err)
-      })
+      .then(this.#getPostResponseFunc(skipPostResponse))
+      .then(handleJSONResponse)
+      .catch(this.#errorHandler(method, path))
   }
 }

packages/@uppy/companion-client/src/Socket.js

@@ -1,83 +1,84 @@
 const ee = require('namespace-emitter')
 
 module.exports = class UppySocket {
-  constructor (opts) {
-    this.opts = opts
-    this._queued = []
-    this.isOpen = false
-    this.emitter = ee()
+  #queued = []
+
+  #emitter = ee()
 
-    this._handleMessage = this._handleMessage.bind(this)
+  #isOpen = false
 
-    this.close = this.close.bind(this)
-    this.emit = this.emit.bind(this)
-    this.on = this.on.bind(this)
-    this.once = this.once.bind(this)
-    this.send = this.send.bind(this)
+  #socket
+
+  constructor (opts) {
+    this.opts = opts
 
     if (!opts || opts.autoOpen !== false) {
       this.open()
     }
   }
 
+  get isOpen () { return this.#isOpen }
+
+  [Symbol.for('uppy test: getSocket')] () { return this.#socket }
+
+  [Symbol.for('uppy test: getQueued')] () { return this.#queued }
+
   open () {
-    this.socket = new WebSocket(this.opts.target)
+    this.#socket = new WebSocket(this.opts.target)
 
-    this.socket.onopen = (e) => {
-      this.isOpen = true
+    this.#socket.onopen = () => {
+      this.#isOpen = true
 
-      while (this._queued.length > 0 && this.isOpen) {
-        const first = this._queued[0]
+      while (this.#queued.length > 0 && this.#isOpen) {
+        const first = this.#queued.shift()
         this.send(first.action, first.payload)
-        this._queued = this._queued.slice(1)
       }
     }
 
-    this.socket.onclose = (e) => {
-      this.isOpen = false
+    this.#socket.onclose = () => {
+      this.#isOpen = false
     }
 
-    this.socket.onmessage = this._handleMessage
+    this.#socket.onmessage = this.#handleMessage
   }
 
   close () {
-    if (this.socket) {
-      this.socket.close()
-    }
+    this.#socket?.close()
   }
 
   send (action, payload) {
     // attach uuid
 
-    if (!this.isOpen) {
-      this._queued.push({ action, payload })
+    if (!this.#isOpen) {
+      this.#queued.push({ action, payload })
       return
     }
 
-    this.socket.send(JSON.stringify({
+    this.#socket.send(JSON.stringify({
       action,
       payload,
     }))
   }
 
   on (action, handler) {
-    this.emitter.on(action, handler)
+    this.#emitter.on(action, handler)
   }
 
   emit (action, payload) {
-    this.emitter.emit(action, payload)
+    this.#emitter.emit(action, payload)
   }
 
   once (action, handler) {
-    this.emitter.once(action, handler)
+    this.#emitter.once(action, handler)
   }
 
-  _handleMessage (e) {
+  #handleMessage= (e) => {
     try {
       const message = JSON.parse(e.data)
       this.emit(message.action, message.payload)
     } catch (err) {
-      console.log(err)
+      // TODO: use a more robust error handler.
+      console.log(err) // eslint-disable-line no-console
     }
   }
 }

packages/@uppy/companion-client/src/Socket.test.js

@@ -15,10 +15,12 @@ describe('Socket', () => {
         webSocketConstructorSpy(target)
       }
 
+      // eslint-disable-next-line class-methods-use-this
       close (args) {
         webSocketCloseSpy(args)
       }
 
+      // eslint-disable-next-line class-methods-use-this
       send (json) {
         webSocketSendSpy(json)
       }
@@ -52,7 +54,7 @@ describe('Socket', () => {
 
   it('should send a message via the websocket if the connection is open', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket.socket
+    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
     webSocketInstance.triggerOpen()
 
     uppySocket.send('bar', 'boo')
@@ -66,17 +68,17 @@ describe('Socket', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
 
     uppySocket.send('bar', 'boo')
-    expect(uppySocket._queued).toEqual([{ action: 'bar', payload: 'boo' }])
+    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([{ action: 'bar', payload: 'boo' }])
     expect(webSocketSendSpy.mock.calls.length).toEqual(0)
   })
 
   it('should queue any messages for the websocket if the connection is not open, then send them when the connection is open', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket.socket
+    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
 
     uppySocket.send('bar', 'boo')
     uppySocket.send('moo', 'baa')
-    expect(uppySocket._queued).toEqual([
+    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([
       { action: 'bar', payload: 'boo' },
       { action: 'moo', payload: 'baa' },
     ])
@@ -84,7 +86,7 @@ describe('Socket', () => {
 
     webSocketInstance.triggerOpen()
 
-    expect(uppySocket._queued).toEqual([])
+    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([])
     expect(webSocketSendSpy.mock.calls.length).toEqual(2)
     expect(webSocketSendSpy.mock.calls[0]).toEqual([
       JSON.stringify({ action: 'bar', payload: 'boo' }),
@@ -96,19 +98,19 @@ describe('Socket', () => {
 
   it('should start queuing any messages when the websocket connection is closed', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket.socket
+    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
     webSocketInstance.triggerOpen()
     uppySocket.send('bar', 'boo')
-    expect(uppySocket._queued).toEqual([])
+    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([])
 
     webSocketInstance.triggerClose()
     uppySocket.send('bar', 'boo')
-    expect(uppySocket._queued).toEqual([{ action: 'bar', payload: 'boo' }])
+    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([{ action: 'bar', payload: 'boo' }])
   })
 
   it('should close the websocket when it is force closed', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket.socket
+    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
     webSocketInstance.triggerOpen()
 
     uppySocket.close()
@@ -117,7 +119,7 @@ describe('Socket', () => {
 
   it('should be able to subscribe to messages received on the websocket', () => {
     const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket.socket
+    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
 
     const emitterListenerMock = jest.fn()
     uppySocket.on('hi', emitterListenerMock)

packages/@uppy/companion-client/types/index.d.ts

@@ -65,7 +65,7 @@ export interface SocketOptions {
 }
 
 export class Socket {
-  isOpen: boolean
+  readonly isOpen: boolean
 
   constructor (opts: SocketOptions)