Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Refactor image uploading #2580

Merged
merged 13 commits into from
Mar 15, 2018
Merged

Refactor image uploading #2580

merged 13 commits into from
Mar 15, 2018

Conversation

brianlovin
Copy link
Contributor

Deploy after merge (delete what needn't be deployed)

  • iris
  • hyperion

Release notes

  • Improves stability of image uploads

@brianlovin brianlovin changed the title Update upload client and server versions Refactor image uploading Mar 14, 2018
@brianlovin
Copy link
Contributor Author

@mxstbr would love your help on something here: for some reason media images are being rendered twice in chat (optimistic update + server response). I've spent quite a while this evening trying to debug in the sendMessage mutation response, but can't figure out what's going on - I think it's because we are matching for duplicates using message.content.body but the optimistic response has a binary file string while the server response has an imgix url, so those don't match.

Otherwise, would love your eye on everything else:

  • Upgraded client and server upload deps
  • Refactored s3 upload utility
  • Fixed the bug where an error in the upload would crash our server (I wasn't rejecting a promise correctly)

One thing I need feedback on:

  • I need to use a temporary /uploads directory to generate a filepath on the server that can then be uploaded to s3. I made it so that these uploads get deleted immediately after being uploaded, but I could see any errors here leading to a buildup of files and a possible mem/storage leak?

@brianlovin
Copy link
Contributor Author

Also note the fs stuff is coming from the example here: https://github.com/jaydenseric/apollo-upload-examples/blob/master/api/resolvers.mjs

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to use a temporary /uploads directory to generate a filepath on the server that can then be uploaded to s3. I made it so that these uploads get deleted immediately after being uploaded, but I could see any errors here leading to a buildup of files and a possible mem/storage leak?

Nice idea in theory. Unfortunately, now doesn't let us access the file system—if you run this code in prod it'll just crash and/or do nothing. How else can we work around that?

@mxstbr
Copy link
Contributor

mxstbr commented Mar 14, 2018

Other than not-using-the-file-system this LG

@brianlovin
Copy link
Contributor Author

Nice idea in theory. Unfortunately, now doesn't let us access the file system—if you run this code in prod it'll just crash and/or do nothing. How else can we work around that?

God damnit. I think s3-image-uploader package does something with a tmp folder that stores the images in memory, but i could be wrong...otherwise I'm not totally sure, we need a way to convert an incoming file stream to a path that can be uploaded to s3.

The s3-image-uploader module doesn't do what we need it to as it can't
handle file streams. The AWS SDK can handle them though, so this patch
switches us to using that directly.

Also removes all that fs code. /cc @brianlovin
@mxstbr
Copy link
Contributor

mxstbr commented Mar 15, 2018

The s3-image-uploader module doesn't do what we need it to as it can't handle file streams. The AWS SDK can handle them though, so this patch switches us to using that directly and it's working perfectly.

I'm trying to repro that optimistic response bug now!

@spectrum-bot
Copy link

spectrum-bot commented Mar 15, 2018

Fails
🚫

These new files do not have Flow enabled:

  • src/components/chatInput/components/style.js
🚫

1 console statement(s) left in iris/models/user.js.

🚫

1 console statement(s) left in iris/mutations/message/addMessage.js.

Warnings
⚠️

These modified files do not have Flow enabled:

  • iris/routes/middlewares/index.js
  • src/components/message/index.js

New dependencies added: aws-sdk and shortid.

aws-sdk

Author: Amazon Web Services

Description: AWS SDK for JavaScript

Homepage: https://github.com/aws/aws-sdk-js

Createdover 5 years ago
Last Updated28 minutes ago
LicenseApache-2.0
Maintainers12
Releases478
Direct Dependenciesbuffer, events, jmespath, querystring, sax, url, uuid, xml2js and xmlbuilder
Keywordsapi, amazon, aws, ec2, simpledb, s3, sqs, ses, sns, route53, rds, elasticache, cloudfront, fps, cloudformation, cloudwatch, dynamodb, iam, swf, autoscaling, cloudsearch, elb, loadbalancing, emr, mapreduce, importexport, storagegateway, workflow, ebs, vpc, beanstalk, glacier, kinesis, cloudtrail and waf
README

AWS SDK for JavaScript

NPM

Gitter chat

Version Build Status Coverage Status

The official AWS SDK for JavaScript, available for browsers and mobile devices,
or Node.js backends

For release notes, see the CHANGELOG. Prior to v2.4.8, release notes can be found at https://aws.amazon.com/releasenotes/?tag=releasenotes%23keywords%23javascript

If you are upgrading from 1.x to 2.0 of the SDK, please see the [upgrading](https://github.com/aws/aws-sdk-js/blob/master/UPGRADING.md) notes for information on how to migrate existing code to work with the new major version.

Installing

In the Browser

To use the SDK in the browser, simply add the following script tag to your
HTML pages:

<script src="https://sdk.amazonaws.com/js/aws-sdk-2.210.0.min.js"></script>

You can also build a custom browser SDK with your specified set of AWS services.
This can allow you to reduce the SDK's size, specify different API versions of
services, or use AWS services that don't currently support CORS if you are
working in an environment that does not enforce CORS. To get started:

http://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/building-sdk-for-browsers.html

The AWS SDK is also compatible with browserify.

In Node.js

The preferred way to install the AWS SDK for Node.js is to use the
npm package manager for Node.js. Simply type the following
into a terminal window:

npm install aws-sdk

In React Native

To use the SDK in a react native project, first install the SDK using npm:

npm install aws-sdk

Then within your application, you can reference the react native compatible version of the SDK with the following:

var AWS = require('aws-sdk/dist/aws-sdk-react-native');

Using Bower

You can also use Bower to install the SDK by typing the
following into a terminal window:

bower install aws-sdk-js

Usage and Getting Started

You can find a getting started guide at:

http://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide

Usage with TypeScript

The AWS SDK for JavaScript bundles TypeScript definition files for use in TypeScript projects and to support tools that can read .d.ts files.
Our goal is to keep these TypeScript definition files updated with each release for any public api.

Pre-requisites

Before you can begin using these TypeScript definitions with your project, you need to make sure your project meets a few of these requirements:

  • Use TypeScript v2.x

  • Includes the TypeScript definitions for node. You can use npm to install this by typing the following into a terminal window:

    npm install --save-dev @types/node
  • Your tsconfig.json or jsconfig.json includes 'dom' and 'es2015.promise' under compilerOptions.lib.
    See tsconfig.json for an example.

In the Browser

To use the TypeScript definition files with the global AWS object in a front-end project, add the following line to the top of your JavaScript file:

/// <reference types="aws-sdk" />

This will provide support for the global AWS object.

In Node.js

To use the TypeScript definition files within a Node.js project, simply import aws-sdk as you normally would.

In a TypeScript file:

// import entire SDK
import AWS = require('aws-sdk');
// import AWS object without services
import AWS = require('aws-sdk/global');
// import individual service
import S3 = require('aws-sdk/clients/s3');

In a JavaScript file:

// import entire SDK
var AWS = require('aws-sdk');
// import AWS object without services
var AWS = require('aws-sdk/global');
// import individual service
var S3 = require('aws-sdk/clients/s3');

With Angular

Due to the SDK's reliance on node.js typings, you may encounter compilation
issues when using the
typings provided by the SDK in an Angular project created using the Angular CLI.

To resolve these issues, either add "types": ["node"] to the project's tsconfig.app.json
file, or remove the "types" field entirely.

Known Limitations

There are a few known limitations with the bundled TypeScript definitions at this time:

  • Service client typings reflect the latest apiVersion, regardless of which apiVersion is specified when creating a client.
  • Service-bound parameters use the any type.

Getting Help

Please use these community resources for getting help. We use the GitHub issues for tracking bugs and feature requests and have limited bandwidth to address them.

  • Ask a question on StackOverflow and tag it with aws-sdk-js
  • Come join the AWS JavaScript community on gitter
  • Open a support ticket with AWS Support
  • If it turns out that you may have found a bug, please open an issue

Opening Issues

If you encounter a bug with the AWS SDK for JavaScript we would like to hear
about it. Search the existing issues
and try to make sure your problem doesn’t already exist before opening a new
issue. It’s helpful if you include the version of the SDK, Node.js or browser
environment and OS you’re using. Please include a stack trace and reduced repro
case when appropriate, too.

The GitHub issues are intended for bug reports and feature requests. For help
and questions with using the AWS SDK for JavaScript please make use of the
resources listed in the Getting Help
section. There are limited resources available for handling issues and by
keeping the list of open issues lean we can respond in a timely manner.

Supported Services

Please see SERVICES.md for a list of supported services.

License

This SDK is distributed under the
Apache License, Version 2.0,
see LICENSE.txt and NOTICE.txt for more information.

shortid

Author: Dylan Greene

Description: Amazingly short non-sequential url-friendly unique id generator.

Homepage: https://github.com/dylang/shortid

Createdabout 6 years ago
Last Updatedabout 6 hours ago
LicenseMIT
Maintainers2
Releases23
Keywordsshort, tiny, id, uuid, bitly, shorten, mongoid, shortid and tinyid
This README is too long to show.

Generated by 🚫 dangerJS

@mxstbr
Copy link
Contributor

mxstbr commented Mar 15, 2018

Can repro that double-images bug, on it.

@mxstbr
Copy link
Contributor

mxstbr commented Mar 15, 2018

I investigated that double-images bug but there's just no way for us to map the local optimistic message to the remote URL. 😕 Any ideas how to fix that?

@brianlovin
Copy link
Contributor Author

I investigated that double-images bug but there's just no way for us to map the local optimistic message to the remote URL. 😕 Any ideas how to fix that?

I have no idea :/ I wish Apollo made this a bit smarter, right? Only thing I can think is to not show an optimistic message at all and just show a loading spinner somewhere while the image uploads, so that when the image appears its a server-saved image.

mxstbr
mxstbr previously approved these changes Mar 15, 2018
@brianlovin
Copy link
Contributor Author

@mxstbr 7cbc914 temporarily hides optimistic media messages and replaces the media uploader icon with a spinner while sending...it's not that bad and will be fine until we can figure out how to just de-dupe the optimistic updates. I'd rather this go on alpha for a day or so before hitting prod.

@brianlovin brianlovin merged commit 9a80e0c into alpha Mar 15, 2018
@brianlovin brianlovin deleted the upgrade-image-uploads branch March 15, 2018 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants