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

Examples update #1389

Merged
merged 15 commits into from
Apr 19, 2019
Merged

Examples update #1389

merged 15 commits into from
Apr 19, 2019

Conversation

samuelayo
Copy link
Contributor

  • Added python flask example
  • Added node.js example

@arturi
Copy link
Contributor

arturi commented Apr 1, 2019

Thank you!

if (req.url === '/upload' && req.method.toLowerCase() === 'post') {
// parse a file upload
var form = new formidable.IncomingForm()

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an upload directory, like form.uploadDir = "./uploads"? Otherwise I ran this on mac and couldn’t figure out where the files went. And maybe also preserve the extension. So:

form.uploadDir = "./uploads"
form.keepExtensions = true

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add some logging, too, like here: https://stackoverflow.com/a/46973498

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the upload dir and the logging. Also added one more example for PHP

@@ -0,0 +1,35 @@
<?php
ini_set('display_errors', 1);
Copy link
Member

Choose a reason for hiding this comment

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

No consistent indentation in this file

ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

if($_SERVER["REQUEST_METHOD"] == "OPTIONS")
Copy link
Member

Choose a reason for hiding this comment

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

Please use triple equality


if($_SERVER["REQUEST_METHOD"] == "OPTIONS")
{
if (isset($_SERVER["HTTP_ACCESS_CONTROL_REQUEST_METHOD"]))
Copy link
Member

Choose a reason for hiding this comment

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

please wrap if contents in {}

exit(0);
}

if($_POST) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change to !empty($_FILES["files"])

if (move_uploaded_file($_FILES["files"]["tmp_name"][0], $target_file)) {
header("Access-Control-Allow-Origin: *");
header('Content-type: application/json');
$data = ["url" => $target_file, "message" => "The file ". basename( $_FILES["files"]["name"][0]). " has been uploaded."];
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent whitespace regarding concatenation here

echo json_encode( $data );
} else {
header("Access-Control-Allow-Origin: *");
header('Content-type: application/json');
Copy link
Member

Choose a reason for hiding this comment

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

Mixing of single & double quotes

header('Content-type: application/json');
$data = ["message" => "Sorry, there was an error uploading your file."];
http_response_code(400);
echo json_encode( $data );
Copy link
Member

Choose a reason for hiding this comment

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

white space within parenthesis is new here

} else {
header("Access-Control-Allow-Origin: *");
header('Content-type: application/json');
$data = ["message" => "Sorry, there was an error uploading your file."];
Copy link
Member

Choose a reason for hiding this comment

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

More descriptive error would be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should use a try catch? and send the error array from the throwable?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that works? I don't think it throws right? So maybe just sth like: Unable to move the uploaded file to its final location: X. Please check up on server permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it throws, using the throwable path. Not sure the move_uploaded_file function throws, as it returns a boolean. I can throw an error from the block when the boolean is false;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it, I guess this way, if any other error that throws comes up, it is also handled

@arturi
Copy link
Contributor

arturi commented Apr 11, 2019

@samuelayo I updated all of the examples to use CSS from the bundle (otherwise we had a mismatch of CSS and JS), tweaked readmes and added uploads folder creation — without it all of the examples didn’t function for me (they errored when trying to write to uploads). Could you let me know if these changes make sense?

I wonder if we should not rely on uppy from the project and instead use the npm version in all those examples. That way you don’t have to run anything in the project root before it’s usable.

@samuelayo
Copy link
Contributor Author

This makes sense @arturi. I also thing we should use the Uppy package from NPM. I'll update the examples to use uppy from NPM.

@arturi
Copy link
Contributor

arturi commented Apr 17, 2019

@samuelayo Problem with that is that it might be out of date with the current package, like after 6 months uppy is 1.3 and examples are 0.30.4, but we can update manually once in a while, I guess.

@samuelayo
Copy link
Contributor Author

@arturi I think so. E.g updating the uppy version after every major release

@kvz
Copy link
Member

kvz commented Apr 17, 2019

@samuelayo Problem with that is that it might be out of date with the current package, like after 6 months uppy is 1.3 and examples are 0.30.4, but we can update manually once in a while, I guess.

This is what we have the 'version replacer' for

@@ -0,0 +1,3 @@
flask
werkzeug
flask-cors
Copy link
Contributor

Choose a reason for hiding this comment

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

probably an extraneous file? python-xhr and php-xhr both have a requirements.txt, should only be python i think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for noting this.

@arturi
Copy link
Contributor

arturi commented Apr 17, 2019

This is what we have the 'version replacer' for

it won’t replace package.json dependencies at the moment, or will it?

@@ -21,6 +16,7 @@
"cors": "^2.8.4",
"formidable": "^1.2.1",
"npm-run-all": "^4.1.3",
"rimraf": "^2.6.2"
"rimraf": "^2.6.2",
"uppy": "^0.30.4"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to add each individual uppy module instead of a bundle thing. So like @uppy/core, @uppy/xhr-upload etc.

@arturi arturi merged commit 3a2dca2 into master Apr 19, 2019
@arturi arturi deleted the examples_update branch April 19, 2019 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants