feat(submission): add uploadProgress subscription resolver #368
Conversation
server/submission/resolvers.js
Outdated
| console.log('new subscription!!') | ||
| setTimeout(() => { | ||
| console.log('published new data') | ||
| pubsub.publish(ON_UPLOAD_PROGRESS, { uploadProgress: 10 }) |
There was a problem hiding this comment.
Since this is the uploadProgress resolver, shouldn't you publish 10 rather than {uploadProgress: 10}?
In the long term, it's probably better to change the schema so uploadProgress returns an object rather than a number, to make it easier to extend.
There was a problem hiding this comment.
You actually have to return it like that. This code is here just to debug and see if it works or not, but I tested this with GraphiQL and after 8 seconds I get back the right stuff and it doesn't complain.
This link has some subscription implementation https://github.com/apollographql/graphql-subscriptions
mihaildu
force-pushed
the
312-push-messages
branch
2 times, most recently
from
890a969
to
18da46a
Compare
mihaildu
force-pushed
the
312-push-messages
branch
from
4b93c36
to
d7d8fe7
Compare
mihaildu
force-pushed
the
312-push-messages
branch
from
4f397f2
to
1c771fb
Compare
mihaildu
changed the title
mihaildu
changed the title
mihaildu
changed the title
| const dropzoneContentWrapper = makeCheerioWrapper({ | ||
| conversion: { converting: true }, | ||
| }) | ||
| expect(dropzoneContentWrapper.text()).toBe(manuscriptUploading) | ||
| }) | ||
|
|
||
| it('displays uploading even if there are errors', () => { |
There was a problem hiding this comment.
Did you mean to skip these? If so, please leave a comment to explain.
There was a problem hiding this comment.
Yes, same as my previous comment, tests needs updating to account for percentage of file upload. Would skip for now.
| @@ -10,6 +14,8 @@ const path = require('path') | |||
| const crypto = require('crypto') | |||
| const Joi = require('joi') | |||
|
|
|||
| const { ON_UPLOAD_PROGRESS } = asyncIterators | |||
There was a problem hiding this comment.
Is it confusing that this ON_UPLOAD_PROGRESS is a string but the other thing called ON_UPLOAD_PROGRESS is a graphql operation?
There was a problem hiding this comment.
Not really, one is client side and the other server side. They had similar examples in apollo as far as I remember.
| const manuscript = await Mutation.uploadManuscript( | ||
| {}, | ||
| { id, file }, | ||
| file.size, |
There was a problem hiding this comment.
Is this right? Shouldn't file size be passed in the second parameter?
There was a problem hiding this comment.
I don't remember what happened here, pretty sure I tested a while ago. This will fail anyway now because you need to subscribe to uploadProgress before you upload a file. I would skip for now and fix the test later since merging this seems like a higher priority.
| {...props} | ||
| /> | ||
| <Subscription subscription={ON_UPLOAD_PROGRESS}> | ||
| {({ data: uploadData, loading: uploadLoading }) => ( |
There was a problem hiding this comment.
What happens if this request fails? Does the progress just stay at zero? If so, I think that's fine. Just would like to make sure it doesn't fail in any more major way.
There was a problem hiding this comment.
I just played with this and if the initial subscription fails then the page does not load at all. The error doesn't seem to be captured in the error render prop as described here https://www.apollographql.com/docs/react/advanced/subscriptions.html#render-prop
…solver Revert "Merge pull request #368 from elifesciences/312-push-messages"
re #312