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

Draft Version Identification #32

Merged
merged 2 commits into from Jan 19, 2022
Merged

Draft Version Identification #32

merged 2 commits into from Jan 19, 2022

Conversation

guoye-zhang
Copy link
Contributor

@smatsson
Copy link
Contributor

smatsson commented Jan 7, 2022

Does this only apply to the 104 and not the other responses? I can see the benefit of adding it to all responses if there are breaking changes between two draft versions.

@guoye-zhang
Copy link
Contributor Author

I think it's more important for feature detection to signal the version since it can fallback to a regular non-resumable upload.

For explicit uses of the resumable upload protocol, finding out version mismatch is too late because it can only lead to failure. Experimental deployments should use different URLs for different draft versions to ensure compatibility. E.g. https://example.com/draft-v1/upload

@Acconut
Copy link
Member

Acconut commented Jan 16, 2022

Interesting proposal. I had thought about a draft version identification as being sent from the client to the server, so the server knows using which version the request should be handled.

In your PR, the identifier is intended, so the client can error out if the server implements a wrong draft version. Is this correct?

@guoye-zhang
Copy link
Contributor Author

I suppose that a request header works too. This is specific to feature detection so incompatible versions ignore each other’s feature detection message

@guoye-zhang
Copy link
Contributor Author

Oh I think a request header is better, since the final version won’t need to worry about this header any more. I’ll update the PR

@guoye-zhang
Copy link
Contributor Author

Updated to have Upload-Draft-Version in both request and 104 response. (It's the only way RFC client and server implementations without this logic can inter-op with draft implementations)

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Sounds great! Thanks for the update!

@Acconut Acconut merged commit 25ef569 into tus:master Jan 19, 2022
@guoye-zhang guoye-zhang deleted the version branch January 20, 2022 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants