Workbook package #3019
Workbook package #3019
Conversation
| let succeeded = false; | ||
| const statusQuery = GraphQueryable(instance, `operations/${opId}`); | ||
| for (let i = 0; i < maxPolls; i++) { | ||
| await new Promise(resolve => setTimeout(resolve, pollIntervalMs)); |
There was a problem hiding this comment.
We have delay() in core you can use for this very thing.
| for (let i = 0; i < maxPolls; i++) { | ||
| await new Promise(resolve => setTimeout(resolve, pollIntervalMs)); | ||
|
|
||
| const status = await graphGet<RichLongRunningOperation>(statusQuery); |
There was a problem hiding this comment.
For GET you should be able to invoke the queryable itself await statusQuery<>()
| instance.using(InjectHeaders({ "Prefer": "respond-async" })); | ||
|
|
||
| instance.on.parse(async function (url, response, result) { | ||
| if (response.status === 202) { |
There was a problem hiding this comment.
What happens if you get a different response? We have some standard logic wrapped up here to check for errors. Used here and here for example.
| WorkbookSessionInfo | ||
| } from "@microsoft/microsoft-graph-types"; | ||
|
|
||
| export async function getWorkbookSession(this: _DriveItem, persistChanges: boolean): Promise<IWorkbookWithSession> { |
There was a problem hiding this comment.
If the only place this will really get used is in the property in the other file you can code it there directly. i.e. remove the export here and put the code there. But if this is a function that will be used on its own this makes sense.
|
1. Your contributing guidelines mention I should skip writing tests for endpoints that don't support app auth. According to the docs, that's all of the workbook endpoints. Is there any way I can write tests we can use for these endpoints? Not really. You can test them locally. We are working on test recording to allow us to record delegated-auth tests and replay them to validate. 2. In my testing, I wasn't able to use workbook operations on items opened by path. These failed with "Could not obtain a WAC token". When I use getItemById, it works. I wrote this off as a Graph API bug, but haven't tested extensively. Don't know, WAC is the office protocol, perhaps that endpoint requires something special. 3. My current implementation of workbook sessions involves a I am not deeply familiar with this API, what is the difference between sessionless and with session? Do we need to expose both? 4. I've added a prefer-async behavior to the graph folder. This is to handle the long-running operation pattern that a few of the workbook endpoints support. Again, I'm not sure if this is the ideal approach, but it's what I've got at the moment. This is pretty slick, I left a few notes inline. This isn't something we built-in for v4 launch, but I can see a ton of value to including it. Again, nice work! |
A few of the collections from the workbook endpoints support getItemAt(index). I've implemented this in the same pattern as the getById decorator. Currently I've only added it to the rows collection.
Category
Related Issues
Mentioned in #3016
What's in this Pull Request?
Initial draft of Excel workbook support.
This first commit includes
I'll implement more endpoints as I go, but have a couple of notes/questions I thought I'd post here first.
workbookproperty on DriveItem that leads to a regular, sessionless IWorkbook and an async methodgetWorkbookSessionthat resolves to an IWorkbookWithSession. The latter is an extension of IWorkbook that adds session refresh/close methods, and uses the InjectHeaders behaviour to add the session id header. Thoughts on this approach?Welcoming all feedback. Thanks!