GSC Datastore: Add support for contentType property #130
Conversation
| @@ -106,6 +106,27 @@ class DataStore extends EventEmitter { | |||
| return resolve({ size: 0, upload_length: 1 }); | |||
| }); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I moved this method here where it can be leveraged by any datastore (not just S3)
There was a problem hiding this comment.
referencing related issue: https://github.com/tus/tus-node-server/pull/89/files#diff-cf34f7e211aecb1cd3ae5f05a5cde2d8R242
tkrugg
force-pushed
the
meta
branch
2 times, most recently
from
50f9d3b
to
ae8be92
Compare
lib/stores/GCSDataStore.js
Outdated
| const options = { | ||
| contentType: parsedMetadata.type.decoded, |
There was a problem hiding this comment.
tus-js-client uses the filetype metadata key to specify the file's type (see https://github.com/tus/tus-js-client#example). I would like it if we only had one property to do so. Would you mind changing it?
Besides that, a check to see if parsedMetadata.type actually exists might be necessary.
There was a problem hiding this comment.
Besides that, a check to see if parsedMetadata.type actually exists might be necessary.
👍 will handle that
tus-js-client uses the filetype metadata key to specify the file's type
It should be possible, let me just check it still works 😄
Answering your global comment about this PR:
it would be nice to have a GCS-specific test to see that this works properly.
I was going to write it but I couldn't get the GCS test suite to run locally. It does run on travis. I may be missing some env variables. Are there a few steps to follow before running them locally?
Thank you very much for taking the time to review this PR. I appreciate.
There was a problem hiding this comment.
@Acconut small reminder about my question:
was going to write it but I couldn't get the GCS test suite to run locally. It does run on travis. I may be missing some env variables. Are there a few steps to follow before running them locally?
There was a problem hiding this comment.
I responded to it in the main thread two days ago: #130 (comment)
lib/stores/GCSDataStore.js
Outdated
| const options = { | ||
| offset, | ||
| metadata: { | ||
| contentType: parsedMetadata.type.decoded, |
There was a problem hiding this comment.
Again, a check to see if parsedMetadata.type actually exists might be necessary.
Yes, there are a few steps to follow. They are not directly documented anywhere but that's on my list to improve. The basic steps should be:
I think that should be all the steps. Let me know if they work for you.
You're welcome. Thank you very much for contribution. |
- using filetype instead of type - added test - handled case when metadata header is undefined
|
@tkrugg Were you able to have a look at the GCS-tests yet? Is this PR ready from your side? |
|
Closing this as stale and the source has now significantly diverged. Thanks for taking the time to contribute. |
closes #57
This PR follows up on #57
I couldn't get test/Test-GCSDataStore.js so i'll trust the travis output to fix the tests