Merge pull request #107 from jaydenseric/deprecated-stream

Added back, but deprecated, the file upload `stream` property.

changelog.md

@@ -5,12 +5,13 @@
 ### Major
 
 - The `processRequest` function now requires a [`http.ServerResponse`](https://nodejs.org/api/http.html#http_class_http_serverresponse) instance as its second argument.
-- `Upload` scalar promises now resolve with a `createReadStream` method instead of a `stream` property, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92).
 - Replaced the previously exported error classes with [`http-errors`](https://npm.im/http-errors) and snapshot tested error details, via [#105](https://github.com/jaydenseric/apollo-upload-server/pull/105).
 - No longer exporting the `SPEC_URL` constant.
 
 ### Minor
 
+- `Upload` scalar promises now resolve with a `createReadStream` method instead of a `stream` property, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92).
+- Accessing an `Upload` scalar promise resolved `stream` property results in a deprecation warning that recommends using `createReadStream` instead. It will be removed in a future release. Via [#107](https://github.com/jaydenseric/apollo-upload-server/pull/107).
 - An `Upload` variable can now be used by multiple resolvers, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92).
 - Multiple `Upload` scalar variables can now use the same multipart data, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92).
 - Malformed requests containing invalid JSON for `operations` or `map` multipart fields cause an appropriate error with a `400` status instead of crashing the process, relating to [#81](https://github.com/jaydenseric/apollo-upload-server/pull/81) and [#95](https://github.com/jaydenseric/apollo-upload-server/issues/95).

src/processRequest.mjs

@@ -1,3 +1,4 @@
+import util from 'util'
 import Busboy from 'busboy'
 import objectPath from 'object-path'
 import WriteStream from 'fs-capacitor'
@@ -267,26 +268,28 @@ export const processRequest = (
 
         stream.pipe(capacitor)
 
-        upload.resolve(
-          Object.create(
-            {},
-            {
-              filename: { value: filename, enumerable: true },
-              mimetype: { value: mimetype, enumerable: true },
-              encoding: { value: encoding, enumerable: true },
-              createReadStream: {
-                value() {
-                  const error = capacitor.error || (released ? exitError : null)
-                  if (error) throw error
-
-                  return capacitor.createReadStream()
-                },
-                enumerable: true
-              },
-              capacitor: { value: capacitor }
-            }
-          )
-        )
+        const file = {
+          filename,
+          mimetype,
+          encoding,
+          createReadStream() {
+            const error = capacitor.error || (released ? exitError : null)
+            if (error) throw error
+            return capacitor.createReadStream()
+          }
+        }
+
+        let capacitorStream
+        Object.defineProperty(file, 'stream', {
+          get: util.deprecate(function() {
+            if (!capacitorStream) capacitorStream = this.createReadStream()
+            return capacitorStream
+          }, 'File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead.')
+        })
+
+        Object.defineProperty(file, 'capacitor', { value: capacitor })
+
+        upload.resolve(file)
       } else {
         // Discard the unexpected file.
         stream.on('error', () => {})

src/test.mjs

@@ -1438,3 +1438,98 @@ t.test('Missing ‘operations’, ‘map’ and files.', async t => {
     await sendRequest(t, port)
   })
 })
+
+t.test('Deprecated file upload ‘stream’ property.', async t => {
+  const sendRequest = async port => {
+    const body = new FormData()
+
+    body.append(
+      'operations',
+      JSON.stringify({
+        variables: {
+          file: null
+        }
+      })
+    )
+
+    body.append('map', JSON.stringify({ 1: ['variables.file'] }))
+    body.append('1', 'a', { filename: 'a.txt' })
+
+    await fetch(`http://localhost:${port}`, { method: 'POST', body })
+  }
+
+  const uploadTest = upload => async t => {
+    const resolved = await upload
+
+    // Store the original process deprecation mode.
+    const { throwDeprecation } = process
+
+    // Allow deprecation warning to be tested.
+    process.throwDeprecation = true
+
+    try {
+      resolved.stream
+      t.fail('No deprecation warning.')
+    } catch (error) {
+      t.matchSnapshot(snapshotError(error), 'Deprecation warning.')
+    }
+
+    // Restore process deprecation mode. The warning won't appear again as
+    // Node.js only displays it once per process.
+    process.throwDeprecation = throwDeprecation
+
+    t.matchSnapshot(JSON.stringify(resolved, null, 2), 'Enumerable properties.')
+
+    t.true(
+      resolved.stream === resolved.stream,
+      'Accessing ‘stream’ multiple times gets the same stream.'
+    )
+    t.type(resolved.stream, ReadStream, 'Stream type.')
+    t.equals(await streamToString(resolved.stream), 'a', 'Contents.')
+  }
+
+  await t.test('Koa middleware.', async t => {
+    t.plan(2)
+
+    let variables
+
+    const app = new Koa().use(apolloUploadKoa()).use(async (ctx, next) => {
+      ;({ variables } = ctx.request.body)
+      await t.test('Upload.', uploadTest(ctx.request.body.variables.file))
+
+      ctx.status = 204
+      await next()
+    })
+
+    const port = await startServer(t, app)
+
+    await sendRequest(port)
+
+    const file = await variables.file
+    await new Promise(resolve => file.capacitor.once('close', resolve))
+    t.false(fs.existsSync(file.capacitor.path), 'Cleanup.')
+  })
+
+  await t.test('Express middleware.', async t => {
+    t.plan(2)
+
+    let variables
+
+    const app = express()
+      .use(apolloUploadExpress())
+      .use((request, response, next) => {
+        ;({ variables } = request.body)
+        t.test('Upload.', uploadTest(request.body.variables.file))
+          .then(() => next())
+          .catch(next)
+      })
+
+    const port = await startServer(t, app)
+
+    await sendRequest(port)
+
+    const file = await variables.file
+    await new Promise(resolve => file.capacitor.once('close', resolve))
+    t.false(fs.existsSync(file.capacitor.path), 'Cleanup.')
+  })
+})

tap-snapshots/lib-test-TAP.test.js

@@ -432,3 +432,33 @@ exports[`lib/test TAP Missing ‘operations’, ‘map’ and files. Express mid
   "expose": true
 }
 `
+
+exports[`lib/test TAP Deprecated file upload ‘stream’ property. Koa middleware. Upload. > Deprecation warning. 1`] = `
+{
+  "name": "DeprecationWarning",
+  "message": "File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead."
+}
+`
+
+exports[`lib/test TAP Deprecated file upload ‘stream’ property. Koa middleware. Upload. > Enumerable properties. 1`] = `
+{
+  "filename": "a.txt",
+  "mimetype": "text/plain",
+  "encoding": "7bit"
+}
+`
+
+exports[`lib/test TAP Deprecated file upload ‘stream’ property. Express middleware. Upload. > Deprecation warning. 1`] = `
+{
+  "name": "DeprecationWarning",
+  "message": "File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead."
+}
+`
+
+exports[`lib/test TAP Deprecated file upload ‘stream’ property. Express middleware. Upload. > Enumerable properties. 1`] = `
+{
+  "filename": "a.txt",
+  "mimetype": "text/plain",
+  "encoding": "7bit"
+}
+`