Allow specifying document ID #142
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
paulgb
force-pushed
the
paulgb/allow-specifying-doc-id
branch
2 times, most recently
from
be0d2c1 to
7292abe
Compare
paulgb
force-pushed
the
paulgb/allow-specifying-doc-id
branch
from
7292abe to
8d86a54
Compare
| @@ -45,3 +43,10 @@ jobs: | |||
| Y_SWEET_S3_BUCKET_PREFIX: testing | |||
| Y_SWEET_S3_BUCKET_NAME: ysweet-testing-y-sweet-data | |||
| working-directory: tests | |||
| timeout-minutes: 10 | |||
|
|
|||
| - uses: actions/upload-artifact@v3 | |||
There was a problem hiding this comment.
this PR adds log redirection to files for debugging (only for the native server, currently). This uploads those to GitHub to make debugging easier.
| @@ -214,8 +219,8 @@ export class DocumentManager { | |||
| * | |||
| * @returns A {@link DocCreationResult} object containing the ID of the created document. | |||
| */ | |||
| public async createDoc(): Promise<DocCreationResult> { | |||
| const result = await this.doFetch('doc/new', { method: 'POST' }) | |||
There was a problem hiding this comment.
This only worked by mistake: doFetch sends a POST request if the body is defined, and this sets the body to {method: POST}. The server ignores the method field.
There was a problem hiding this comment.
I think I'd vote for just having doFetch accept a method (like fetch() does) rather than having to pass an empty object as the body in order to "hack around" the faulty heuristic of setting the method based on the body. Thoughts?
There was a problem hiding this comment.
Also, I think the getClientToken() method below is using doFetch() incorrectly (passing what looks like FetchOptions as the request body).
There was a problem hiding this comment.
My general principle for the API is that every POST request should have a valid JSON body, even if it is an empty object. Since doFetch is not a general-purpose fetch function but one designed for this API, the GET/POST distinction restricts the degrees of freedom that the caller has to break things.
There was a problem hiding this comment.
Also, I think the
getClientToken()method below is usingdoFetch()incorrectly (passing what looks like FetchOptions as the request body).
Good point, here's a fix: #145
There was a problem hiding this comment.
I'm not sure it restricts degrees of freedom for the caller. After all, the caller of doFetch() can still break things by forgetting to pass an empty object.
My main issue is that it's just easy for the caller of doFetch to make bad assumptions about what doFetch will do. At this point in the logic, all the dev cares about are the 3 main components of a REST endpoint: method, path, and request body. Seems to me that designing a doFetch interface that looks like that is more likely to result in a dev correctly predicting what the code will do.
There was a problem hiding this comment.
Are you thinking that the signature should be doFetch(url, method, body?)? I guess I could use typescript overloads to ensure that a body is passed if and only if the method is POST.
Alternatively, I could use doGet(url) and doPost(url, body), although they'd still use a shared underlying doFetch, so we could still shoot ourselves in the foot by calling the underlying doFetch instead of doGet or doPost.
| @@ -1 +1,2 @@ | |||
| node_modules | |||
| out | |||
There was a problem hiding this comment.
Tests now create some output here for debugging.
There was a problem hiding this comment.
nit: can we call this test-output or something like that?
| }), | ||
| ).rejects.toThrow('400') | ||
| }) | ||
|
|
There was a problem hiding this comment.
What should happen when creating a doc with a name that's already been used? Is that a 409 Conflict?
There was a problem hiding this comment.
I think it should just return the same document. This way document creation is idempotent.
In some cases, a developer may already be managing unique IDs (e.g. sequential or UUIDs) for a set of objects, and would like to map these to y-sweet documents. They could maintain a mapping from their own IDs to y-sweet document IDs, but it's easier for us to just allow them to provide an ID when they create the document.
I think this was suggested by @colelawrence, and it also relates to (but does not close) #141.