search

pnp / pnpjs

Fluent JavaScript API for SharePoint and Microsoft Graph REST APIs
https://pnp.github.io/pnpjs/
Other
742 stars 302 forks source link

Workbook package #3019

Open ChapC opened 2 months ago

ChapC commented 2 months ago

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.

  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?
  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.
  3. My current implementation of workbook sessions involves a workbook property on DriveItem that leads to a regular, sessionless IWorkbook and an async method getWorkbookSession that 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?
  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.

Welcoming all feedback. Thanks!

patrick-rodgers commented 2 months ago

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 workbook property on DriveItem that leads to a regular, sessionless IWorkbook and an async method getWorkbookSession that 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?

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!