Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds koa route #80

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

OKNoah
Copy link

@OKNoah OKNoah commented Aug 20, 2016

No description provided.

OKNoah added 3 commits August 19, 2016 17:45
Did not understand how they worked before
@@ -25,9 +25,10 @@
"homepage": "https://github.com/odysseyscience/react-s3-uploader",
"dependencies": {
"aws-sdk": "2.x",
"koa-router": "^5.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not familiar with koa, but notice we took out the express dependency in this file, and are just relying on people have it installed if they are using the bundled router. That keeps it so that people not using express (or in this case koa) don't have to download the dependency.

Is koa a large library? Does it have a lot of dependencies? If so, we should probably remove it from this file.

Copy link
Author

Choose a reason for hiding this comment

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

Koa would need to be in the server.js (presumably) already to work. You'll do

const app = koa()

app.use(s3routes(options))

No real way to not already have koa.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using this router, the assumption would be that you already have a dependency on koa. But if you aren't using this router, you don't want the koa dependency, and you would get it if this line exists in package.json. So we should delete this line from here I think. Make sense?

@seanadkinson
Copy link
Contributor

The options you added (endpoint and prefix) could be useful in the existing router. Still would like to hear how the existing router didn't work for koa users before merging this.

Thanks! 👍

@OKNoah
Copy link
Author

OKNoah commented Aug 22, 2016

@seanadkinson Express routers don't work at all in Koa. Koa to is similar to Express (same devs), but uses generators, and instead of req and res, it uses this.

There are a few other differences in here I should fix, like I took out the forward slash check on the key for some reason. But if there's interest I'll clean it up. Also the /s3/img stuff should be tested by someone else since I don't really use that in my app.

@seanadkinson
Copy link
Contributor

Yeah, I think we can merge. Ideally this package is split up into s3-uploader-react and s3-uploader-express (and s3-uploader-koa with this addition), but I just haven't gotten around to it, nor do I think I will anytime soon.

So we can just provide the router code that someone may or may not use, if you'd like to make the couple changes you mentioned. The main thing I want is to not download the koa package if I'm not using it, so just cleaning up package.json. Side note, I think optionalDependencies is where you could specify express and koa in these cases, meaning don't download it, but people should have it if they use certain features of the package.

@ktonon
Copy link

ktonon commented Feb 2, 2017

@OKNoah Has any more work been done on this? I'm in favour of splitting this out into 3 packages as @seanadkinson suggested. I need this for a project, so I'm going to take the work you did and publish it separately. I'll reference this PR to give you credit

@ktonon
Copy link

ktonon commented Feb 2, 2017

Published as a separate package: https://www.npmjs.com/package/koa-s3-sign-upload
with some modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants