Refactor and async/await

[mifi]

This PR is for changes related to #85

Other future improvements moved here: #88 #89

Breaking changes ‼️

I have made some changes to make the API more consistent and easy to understand. This is an almost complete rewrite so there may be some unforeseen changes. These are the known breaking changes:

• All previous callback methods now return a promise and do not accept a callback
• replayAssembly(opts) changed to replayAssembly(assemblyId, params) (previously assemblyId was a key inside opts, but it was not optional, it was required)
• replayAssemblyNotification(opts) changed to replayAssemblyNotification(assemblyId, params) (previously assemblyId was a key inside opts, but it was not optional, it was required)
• deleteAssembly renamed to cancelAssembly to reflect the official terminology
• Removed undocumented fields option (directly under createAssembly(opts)). Please use the fields key inside params instead.
• Increase default request timeout from 5000 to 60000
• Changed createAssembly(options[, onProgress]) to: createAssembly({ onUploadProgress: Function({ uploadedBytes, totalBytes }), onAssemblyProgress: Function(assembly) }) (see readme)
• return from waitForCompletion with the assembly result instead of throwing unknown error if result.ok is ASSEMBLY_CANCELED or REQUEST_ABORTED

Declarative createAssembly

addFile and addStream have been removed and are instead part of createAssembly:

// Before:
transloadit.addFile('file1', '/path/to/file')
transloadit.createAssembly({ ... })

// Now:
transloadit.createAssembly({
  files: {
    file1: '/path/to/file'
  },
  ...
})

// Before:
transloadit.addStream('file2', process.stdin)
transloadit.createAssembly({ ... })

// Now:
transloadit.createAssembly({
  uploads: {
    file2: process.stdin
  },
  ...
})

Auto retry logic

• Will now only auto retry 5 times on RATE_LIMIT_REACHED (previous retry logic was overly aggressive: Used to retry on almost all errors, even unrecoverable ones, e.g. INVALID_FILE_META_DATA). Relevant code: 1 2
• Will no longer automatically retry if assembly_url == null or assembly_ssl_url == null. Will instead throw a TransloaditClient.InconsistentResponseError. (relevant code)

Errors

Thrown errors have changed:

When HTTP response code is not successful, the error will now be a TransloaditClient.HTTPError object with an additional transloaditErrorCode property (used to be a normal Error object)

• Message has been improved
• Error property error has been renamed to transloaditErrorCode
• Error property assembly_id has been renamed to assemblyId
• All other JSON response properties from the Transloadit JSON response are no longer added directly to the Error object, but can instead be found in HTTPError.response.body.
• The rest of the response JSON properties can be found on under HTTPError.response?.body (e.g. catch (err) { err.response.assembly_id }) - Note that err.response will be undefined for non-server errors
• Will now also await ASSEMBLY_REPLAYING when waitForCompletion
• Assemblies that have an error status (assembly.error) (but HTTP 200) will now result in an error thrown also when calling replayAssembly, to make it consistent with createAssembly
• No longer throwing "Unknown error" for createTemplate and editTemplate if result.ok happens to be not defined
• 404 response from the server will now throw a TransloaditClient.HTTPError (previously 404 gave a successful result)

See also README

All finished changes

• Create typescript typings file
• promise API signature
• async/awaitify internal code
• simplify flow
• reduce callback hell (deep nesting)
• replace request with got
• fix broken upload progress code
• migrate from travis to github actions
• fix race condition with this._streams
• Fix broken integration test (they did not all run before this PR) and make more easier to read (async/await)
• Error when using a non-file stream Error when using a non-file stream #86
• Remove transpiling
  • Set package.json.engines to "node" : ">=10.0.0" (current version run with tests)
• Merge .eslintrc and .eslintrc.js
• update docs / examples
• implement progress for non-tus (form) uploads too (currently progress only works for TUS)
• async/awaitify PaginationStream
• upgrade tus client to latest version (has breaking changes)
• Why do we need to retry here? I believe we shouldn't blindly retry all errors:

  node-sdk/src/TransloaditClient.js

  Line 647 in 9bb622e

  if (operation.retry(err)) {

• Better error handling:

  node-sdk/src/TransloaditClient.js

  Line 710 in 9bb622e

  return cb(_.extend(new Error(), result, extendedMessage))

  • Store response body in error object instead of spreading it directly on the Error object? If response body ever contains for example the property "code", this may cause a bug
  • Document errors better
• Why is this needed? (404 and 599 check)

  node-sdk/src/TransloaditClient.js

  Line 705 in 9bb622e

  if (statusCode !== 200 && statusCode !== 404 && statusCode >= 400 && statusCode <= 599) {

• Remove console.log, console.warn etc? use debug module?
• FIXME uses private internals of node-retry

  node-sdk/src/TransloaditClient.js

  Line 619 in 9bb622e

  // FIXME uses private internals of node-retry

  • Replace with promise based retry?
  • Fix retrying so that it only retries when the API tells us, and obey API throttle parameter (https://transloadit.com/blog/2012/04/introducing-rate-limiting/)
• allow specifying request timeout? Currently default timeout is 5 sec except createassembly which is 24h
• allow specifying max retries for RATE_LIMIT_REACHED (or disable retry)?
• add an arbitrary string or buffer as a file upload
• assembly_ssl_url in error message - useful for debugging
• Remove existing callback signature and/or remove Async prefix to all methods (breaking change). If not, use callbackify. Depracate callback strategy?
  • Make tests use async/await (and make them more readable)
  • Warn users if calling createAssembly with a callback function now
• Doesn't API give non-200 response if response/assembly has error? If it doesn't, should we do this for all requests for consistency?

  node-sdk/src/TransloaditClient.js

  Line 151 in 9bb622e

  err = new Error(result.error)

• "Unknown error": Consistently throw this for all API calls that return an unexpected ok status? Maybe throw a custom error type InconsistentResponseError?

  node-sdk/src/TransloaditClient.js

  Line 13 in 9bb622e

  function unknownErrMsg (str) {

README.md / documentation

• Make API docs in readme more up to date. make up to date or refer more to https://transloadit.com/docs/api/ ?

Tests

• Integration test still fails: should trigger progress callbacks when uploading files
• Improve slow test (uses a lot of resources, what's the point of this test?)
• rewrite to Jest
• run integration tests automatically. they were really broken with the original code, so I suspect they had not been run for a long time
  • auto run integration tests only on master branch, to keep credentials secret and cost low
  • run unit tests on PR
• show coverage badge
• better test coverage
  • integration tests for file & non-file streams
  • test retry logic
  • Untested:
    • editTemplate
    • listTemplates
    • getBill
    • listAssemblyNotifications
  • see coverage

Declarative API

Suggested change of the API (breaking). Example:

// Set up some different file sources:
const svgStr = '<?xml version="1.0" standalone="no"?><svg height="100" width="100"><circle cx="50" cy="50" r="40" fill="red" /></svg>'
const buffer = Buffer.from(svgStr, 'utf-8')
const streamedJpg = require('got').stream('https://demos.transloadit.com/66/01604e7d0248109df8c7cc0f8daef8/snowflake.jpg')

With existing API:

transloadit.addFile('file1', '/path/to/file.jpg')
transloadit.add('file2', streamedJpg)
transloadit.add('file3', svgStr)
transloadit.add('file4', buffer)
// We now have state. If we accidentally forgot that we added something, could cause bugs

const results = await transloadit.createAssembly({
  params: {
    steps: { ... },
  }
  waitForCompletion: true,
})

With proposed new declarative API:

const results = await transloadit.createAssembly({
  files: {
    file1: '/path/to/file.jpg'
  },
  uploads: {
    file2: streamedJpg, // auto detects that it's a stream
    file3: svgStr, // auto detects that it's a string
    file4: buffer // auto detects that it's a buffer
  },
  params: {
    steps: { ... }
  },
  waitForCompletion: true
})

Advantages of existing API:

• Easy for imperative code using for-loop (for y of arr: transloadit.addFile(x, y))
• No breaking change
• ...

Advantages of declarative API:

• Allows the whole operation to be specified as a JSON object
• Easy for functional code
• No concurrency issue when resetting internal state (See discussion below)
• ...

Cleanup

• remove badge gist (now using a separate github pages repo for reports and badge instead)

[mifi]

Tests broke when replacing request due to different form handling. Will look into that

[kvz]

👌I think if we push the current master to a V2 branch, and supply hotfixes via that, we can merge your new work against master each time

[kvz]

I think it would be cool to release an rc1, and dogfood it in one of our projects. For instance,

• our statuspage which creates Assemblies every minute or so, to test our service. Or:
• our content repo which uses it to generate /demos/. Or:
• Transloadify. Which aims to be a user friendly CLI wrapper around the node-sdk and has some extra batteries included like monitoring a directory for incoming files, transcoding via Transloadit, and writing results to a different folder

Then we'll be able to drive out some early bugs and give more feedback before releasing the final major.

[tim-kos]

Great work, Mikael! I couldn't spot anything that stuck out badly. But it's a huge PR and it's already late, so bugs will likely be in there.

Very grateful for this! Agreeing with @kvz on the dogfooding and deployment strategy.

[juliangruber]

Declarative API

I'm for the declarative API, it's simple and works 👍 Also it's more obvious where state is coming from.

Downside: If we adapt our internal api2 test helper to match this api, we need to rewrite a lot of tests.

[kvz]

I feel for declarative too.

Downside: If we adapt our internal api2 test helper to match this api, we need to rewrite a lot of tests.

Options:

• codemod to rewrite the tests
• also support imperative
• keep using old node-sdk
• ?

[juliangruber]

• also support imperative

The current imperative API is actually dangerous:

const transloadit = ...
for (const item of items) {
  transloadit.addFile('foo', 'bar')
  transloadit.createAssembly()
}

It's dangerous because transloadit actually carries the meaning of assembly. If you want to create multiple assemblies, you need to create multiple transloadits. Therefore I'm strongly in favor of hard dropping the old api.

[kvz]

It's dangerous because transloadit actually carries the meaning of assembly. If you want to create multiple assemblies, you need to create multiple transloadits. Therefore I'm strongly in favor of hard dropping the old api.

oh that's right, i think someone actually hit that

[mifi]

It's dangerous because transloadit actually carries the meaning of assembly. If you want to create multiple assemblies, you need to create multiple transloadits. Therefore I'm strongly in favor of hard dropping the old api.

I think the culprit is this:

node-sdk/src/TransloaditClient.js

Line 148 in 9bb622e

this._streams = {}

You can see that it is being called AFTER remoteJson finishes (which is definitely not in the same tick)

However in the new rewrite I think it should be safe to do what you did, because I reset this.streams very quickly (in the same tick):

node-sdk/src/TransloaditClient.js

Line 182 in f7e52b7

this._streams = {}

However if we want to keep the imperative API, then I believe we should definitely write a unit test for that corner case of creating multiple assemblies without awaiting between!

[mifi]

Would be cool if we can get a decision on this fast so I can merge this and release the RC1 so that people can start using and testing it.

[mifi]

Oh wait, it is not safe with my code either, because I call an await before that.

node-sdk/src/TransloaditClient.js

Line 168 in f7e52b7

await access(path, fs.F_OK | fs.R_OK)

But I can easily pull the this._streams = {} to the top of the function and add a comment that it must be kept on the top

[mifi]

Have now fixed the issue and added a test for it, so it should be safe now.

An option to not have to rewrite a lot of tests if we want to have a declarative API:
Maybe we can make a thin wrapper class that translates the imperative API (and possibly old callback signature) to the node-sdk v3 declarative. Then the tests can just replace an import of TransloaditClient with an import of the wrapper class.
This wrapper class could be exported by node-sdk (undocumented/private API) or could be kept somewhere else.

[kvz]

Best option so far!

[mifi]

Have now made the API declarative and created a backward compatible TransloaditClientLegacy

So in theory we can now do this in old code:

-const TransloaditClient = require('transloadit')
+const TransloaditClient = require('transloadit/src/TransloaditClientLegacy')

Should work roughly like the old API. Note that certain things like Errors and retry logic is different.