Amplify-js: Pre-PR discussion: upload progress for Storage class

[ffxsam]

Hi all,

I've whipped up a modification to Storage.put that I wanted to run by the team before opening a PR. What do you think of the below? (also, see below the code for some questions)

@mlabieniec @mbahar

    public async put(key: string, object, options?): Promise<Object> {
        const credentialsOK = await this._ensureCredentials();
        if (!credentialsOK) { return Promise.reject('No credentials'); }

        const opt = Object.assign({}, this._options, options);
        const { bucket, region, credentials, level, track, progressCallback } = opt;
        const { contentType, contentDisposition, cacheControl, expires, metadata } = opt;
        const type = contentType ? contentType : 'binary/octet-stream';

        const prefix = this._prefix(opt);
        const final_key = prefix + key;
        const s3 = this._createS3(opt);
        logger.debug('put ' + key + ' to ' + final_key);

        const params: any = {
            Bucket: bucket,
            Key: final_key,
            Body: object,
            ContentType: type
        };
        if (cacheControl) { params.CacheControl = cacheControl; }
        if (contentDisposition) { params.ContentDisposition = contentDisposition; }
        if (expires) { params.Expires = expires; }
        if (metadata) { params.Metadata = metadata; }

        try {
            const upload = s3
                .upload(params)
                .on('httpUploadProgress', ({ loaded }) => {
                    if (progressCallback) {
                        progressCallback(loaded);
                    }
                });
            const data = await upload.promise();

            logger.debug('upload result', data);
            dispatchStorageEvent(
                track,
                { method: 'put', result: 'success' },
                null);

            return {
                key: data.Key.substr(prefix.length)
            };
        } catch (e) {
            logger.warn("error uploading", e);
            dispatchStorageEvent(
                track,
                { method: 'put', result: 'failed' },
                null);

            throw e;
        }
    }

Some questions that came to mind while I was working on this:

1. If we're using TypeScript, we should use it to its fullest extent. Why aren't we using an interface, for example, for the options parameter here?
2. If we're using async/await, we might want to start moving towards using it universally (rather than Promise) and using try/catch blocks instead of an (err, res) => {} callback, just for the sake of code legibility.

[ffxsam]

@mlabieniec @mbahar Thoughts?

[mikecousins]

I like this a lot. We need this functionality so that we can start uploading directly to S3 instead of through our app server.

Have you tested this in various browsers? I've noticed that many browser/os combos report incorrect upload stats. Namely Windows/Chrome which is our most popular combo. The browser reports it being done when it's actually only 5-10% done. I'm assuming behind the scenes it's reporting progress as it passes data from the browser to the OS or something, instead of actually over the pipe.

Reference:

https://github.com/visionmedia/superagent/issues/1236
https://github.com/axios/axios/issues/593

Update:

I actually just tested this again since I haven't in a couple years. It now works properly in Windows/Chrome and Windows/IE11. Windows/Edge is still broken and we only get an event firing at the end but there is a bug open for that and supposedly it's fixed in the next release: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12224510/

The crappy part is I don't know if this was fixed in Chrome/IE11 or if it was fixed in Windows itself, and then I don't know in what version of Windows it was fixed. Until we have most of our users in a good state I think we'll have to continue uploading through our app server and using web sockets to push progress back to the clients.

[mlabieniec]

1. I agree, but I think this is part of a larger cleanup task. Some of this is tech debt from early on and can/will be refactored to better support types/interfaces in the coming months (I have an item in our backlog for this)

2. All of the lib modules have Promise returns, and I think if we were to change this, it would be a larger architecture design decision we would have to look at. Personally, if we were going to move/refactor for something like this I'd prefer leaning towards an observer pattern with observables perhaps with the zen-observable we have as a dep in PubSub. What do you think?

Regarding #2, we could potentially use observables here for this instead of sending in the call back. I think this solution proposed here however technically works though for simple progress events.

@haverchuck @jordanranz @elorzafe what do you think on the progress implementation here?

[ffxsam]

Personally, if we were going to move/refactor for something like this I'd prefer leaning towards an observer pattern with observables perhaps with the zen-observable we have as a dep in PubSub. What do you think?

Seems overkill in this particular scenario. I know observables are hot right now. 😉 But I think in this case, async/await is perfectly sufficient.

All of the lib modules have Promise returns, and I think if we were to change this

Nothing is being changed here about what the function returns. Since async functions return a Promise under the hood, I think we're fine. In fact, in the current code, return new Promise within this function is redundant.

So I would say the above code is exactly what I'd submit in the PR. There are no breaking changes here.

[mlabieniec]

I thought they were hot 2 years ago haha ;)

I was more just referring to the observer design pattern in general, which is generally what i think works well in these situations, and what we tend to lean towards. But either way, your proposal here does work fine and I think this is technically feasible for simple progress here. Feel free to PR it and we can run through the review with the team there. :)

[haverchuck]

This looks great to me!

[elorzafe]

The code looks good, why only loaded and not total bytes? (I would use the same object without the destructuring)

[ffxsam]

@elorzafe Sure, I can add that in in my PR. I left it out because the total file size should already be known. But it wouldn't hurt to include it in the callback.

[ffxsam]

@mlabieniec I need some help. I'm running into some errors when I try to commit my changes:

$ tslint 'src/**/*.ts' && jest --coverage
Warning: The 'no-boolean-literal-compare' rule requires type information.
● Deprecation Warning:

  Option "mapCoverage" has been removed, as it's no longer necessary.

  Please update your configuration.

  Configuration Documentation:
  https://facebook.github.io/jest/docs/configuration.html

 FAIL  __tests__/Credentials-test.ts
  ● Credentials test › get test › credentials in the memory and not expired

    TypeError: Cannot read property 'identityPoolId' of undefined

      246 |     private _loadCredentials(credentials, source, authenticated, info): Promise<ICredentials> {
      247 |         const that = this;
    > 248 |         const { identityPoolId } = this._config;
      249 |         return new Promise((res, rej) => {
      250 |             credentials.get(async (err) => {
      251 |                 if (err) {

There's nothing in https://github.com/aws-amplify/amplify-js/blob/master/CONTRIBUTING.md that explains any kind of special setup I need, but I feel like there maybe should be.

[elorzafe]

@ffxsam did you pull the latest code?

[ffxsam]

That fixed it. Pulled upstream master to my remote, rebased my feature on top of that, ran yarn. Good to go!

PS: I got a warning about having both yarn.lock and package-lock.json. We probably shouldn't have both, and then it should be enforced in CONTRIBUTING.md to use either npm or yarn.

[ffxsam]

PR opened, awaiting review.

[elorzafe]

Thanks @ffxsam we are closing this issue, because the PR was already merged and published. Thank you for your contribution!