Cannot read property 'Content-Type' of undefined

[RoelVB]

Major Version

3.x

Minor Version Number

3.24.0

Target environment

NodeJS

Additional environment details

When adding a graph delete request to a batch, we get the error: TypeError: Cannot read property 'Content-Type' of undefined

Expected or Desired Behavior

No error is expected, since the request was successful

Observed Behavior

The error occurs in the parseReponse() method in the batching file: https://github.com/pnp/pnpjs/blob/0240d02f1f1b6037d8c234ec5606cbb4bb8ccad1/packages/graph/batching.ts#L378

The response we get has HTTP code 204 and doesn't contain any headers:

[juliemturner]

The release you're referencing is very stable, so I suspect something you have set up isn't correct. I can't really help you without any code showing what you're doing and what line the error is happening on. I'm going to guess that you don't have any behaviors set up... but again just a shot in the dark without any further information.

[RoelVB]

Hi @juliemturner Thank you for the prompt response. I'm sorry I didn't have time to create a short code to reproduce this issue. As it occurs as part of a complex piece of code.

I've worked out a compact example which causes the error:

import { graphfi } from '@pnp/graph';
import '@pnp/graph/teams';
import '@pnp/graph/batching';
import { GraphDefault } from '@pnp/nodejs';

const graph = graphfi().using(GraphDefault({
    baseUrl: 'https://graph.microsoft.com',
    msal: {
        config: {
            auth: {
                clientId: '<CLIENTID>>',
                clientSecret: '<CLIENTSECRET>>',
                authority: '<AUTHORITY>',
            },
        },
        scopes: [ 'https://graph.microsoft.com/.default' ]
    }
}));

// The MSTeam ID we're going to test with
const TESTTEAM = '<SOME GUID>';

(async ()=>{
    // Get the first channel from the Team (there seems to be a typing issue here, so we cast it to any for now)
    const testChannel: any = (await graph.teams.getById(TESTTEAM).channels())[0];
    console.log(`Testing with channel "${testChannel.displayName}"`);

    // Create a new tab
    const newTab = await graph.teams.getById(TESTTEAM).channels.getById(testChannel.id).tabs.add(
        'PnPJS Test',
        'https://graph.microsoft.com/v1.0/appCatalogs/teamsApps/com.microsoft.teamspace.tab.web',
        {
            configuration: {
                contentUrl: 'https://www.google.com',
                websiteUrl: 'https://www.google.com',
            }
        }
    );

    // Remove the created tab in a batched request
    const [batch,execute] = graph.batched();
    batch.teams.getById(TESTTEAM).channels.getById(testChannel.id).tabs.getById(newTab.data.id).delete();

    try {
        await execute();
    } catch(error) {
        console.error('Something went wrong.', error); // We the "Cannot read property 'Content-Type' of undefined" error here
    }
})();

This code creates a tab in a Teams channel and successfully removes it, but still throws an error. Calling the delete() outside of the batch works without any error.

[juliemturner]

Ok, so to summarize the delete teams tab call works but doesn't work batched? That's a distinct possibility because not all calls work batched, but it may be something we can fix. I'll look into it further.

[RoelVB]

@juliemturner No, the call is successful, but PnPJS throws an error, because it tries to get a response header that isn't there. The batched call returns HTTP 204 and the tab is actually deleted.

In this case, response.headers is undefined at this point: https://github.com/pnp/pnpjs/blob/0240d02f1f1b6037d8c234ec5606cbb4bb8ccad1/packages/graph/batching.ts#L378

Also... the tab deletion is an example. I've seen it with other batched deletes.

[juliemturner]

Yes, we've confirmed that this is a bug in that we don't support calls that return 204 in batches, we'll update this to fix it in a future release.

[RoelVB]

I would gladly provide a PR, but I'm not sure what would be the desired way to fix this. Quick and dirty would be just changing the line mentioned to:

const contentType = response.headers?.["Content-Type"]; 

[juliemturner]

This fix has been made for V4, not v3. I don't think there's a reason to fix v3 given that there's no impediment to moving to v4 but if you have some specific reason why you need to stay in v3 please let us know.

[RoelVB]

Hi @juliemturner

I don't know how we can move to v4 with the change Add/Update methods no longer returning data and a queryable instance. Not having add/update return something will break our software in many ways.

I also don't really understand how that would work. For example we create an item:

await sp.web.list.getByTitle("My List").items.add({Title: "An example"});

How do we get the ID of the item we've just created if there's no return value?

If you don't mind, I've created PR #3052 for v3.

[juliemturner]

That's not a barrier to you moving to v4. What we mean by that update is that in v3 the add and update events returned an object with data and item { data: [result of the add/update command], "item": [a queryable for that new item] } In v4 we are only directly returning the result of the add/update command. So, you're going to get the id of the item you just created you'll just have to refactor your add/update call results to not reference the id via .data. Pretty straight forward, I think.

Releasing v3 builds at this point are very labor intensive and given we went forever without anyone bringing this up it feels not especially needed. If you have other concerns that you think prevent you from moving to v4 please let us know. We'll discuss the processes of doing this update in v3 in our next team meeting and decide if we feel we want to move forward.

[RoelVB]

Thank you for clarifying the v4 transition.

I was under the assumption the v3 builds are largely automated. I'm not going to expect you to keep maintaining v3 when v4 has very few breaking changes.

For me transitioning to v4 will still be very labor intensive. I'm using some quite complex wrapper classes and extensions. In this case, for me, it would currently make most sense to use a forked version including the fix.

---

Also... things like this are going to make the transition even more difficult: https://github.com/pnp/pnpjs/blob/8bcaffd941d4786bad7c486ca980c71b378a888f/packages/sp/items/types.ts#L115-L117 Since the returntype is any, after transitioning, we're only going to detect there's an issue here during runtime.

[juliemturner]

I was under the assumption the v3 builds are largely automated.

Yes, for the current release version this is true, but not for the older versions as we have to decouple it. It can be done, we just want to make sure there's a good reason to do so.

Since the returntype is any, after transitioning, we're only going to detect there's an issue here during runtime.

I find this confusing comment as the returntype for the data has always been any. This is the typing for IItemAddResult in v3, note data is type any....

export interface IItemAddResult {
    item: IItem;
    data: any;
}

[RoelVB]

As for my example. This would be my v3 code:

const newItem = await list.items.add({Title: 'Mylist'});
await newItem.item.update({AdditionalProperty: 'More info'});

If I would do the same thing in v4, the IDE and transpiler would not complain, because newItem is now of type any. So newItem.item.update() is still valid.

I understand that will just be how it is, since in v3 newItem.date.item.update() would also be accepted. But this makes migrating a little more challenging.

---

Theoretically this could be caught using a generic type like this, but I understand that's far from ideal:

public async add<T = {Id: number}>(properties: Record<string, any> = {}): Promise<T> { 
  return spPost(this, body(properties)); 
} 

Going even further it could be something like this:

type ItemValue = string|boolean|number|null|{[key: string]: ItemValue};

interface IItem {
    Id: number;
    [key: string]: ItemValue;
}

async function add<T extends IItem = IItem>(properties: Record<string, any> = {}): Promise<T> { 
  return spPost(this, body(properties)); 
} 

This is a quick writeup to illustrate the idea, this code is already flawed and I'm not even sure if there is a acceptable way of implementing this.

[juliemturner]

We'll consider your suggestion, in the mean time I spent the time and reworked our release solution so I could release v3, you'll find your PR in version 3.25.0.

[bcameron1231]

@RoelVB Could you not just type your response value?

const item:MyCustomType = await sp.web.list.getByTitle("My List").items.add({Title: "An example"});

//assuming MyCustomType doesn't have item property, this would fail. console.log(item.item) //would show typescript error

[RoelVB]

Thank you @juliemturner, I really appreciate it!

@bcameron1231 Of course I could, but that's not the point. I really dislike a method returning any, but that's maybe just a "me problem" 😊

[juliemturner]

I quickly did a test to verify my and @bcameron1231 thoughts on this and am convinced any is the right call here as it provides the most flexibility. Note the screenshot below, linting is happy, and I think it's fine with our any return because our library is in node_modules it's usually ignored for linting rules (this was tested with default rules for new SPFx 1.19.0 project -- your personal setup may vary).

If we type the return as you suggested than there is no easy way to type the returned value specifically. For myself, and many others, we create an object specifically to the list item we expect to be returned. If we return an IItem type, the only way to manage it is to coerce the return value to unknown and then back to my specific type (line 37).

Your possible best option (assuming you don't have typed interfaces for the objects you expect to return) might be is to define your IItem type as you did and then use that as the type for the return from .add or .update calls (see line 41). That solves your problem and leaves the library flexible enough for everyone. Although that said, if you note line 42 I was able to just set a untyped constant to the result of add and it was fine, but maybe you have some other linting rules I am not aware of.

[RoelVB]

I personally think a return type of Record<string, any> makes at least more sense than any.

---

And for my type addiction I came up with this example:

declare module '@pnp/sp/items'
{
    interface IItems
    {
        add<T = {Id: number}>(properties?: Record<string, any>): Promise<T>;
    }
}

This way I get errors all over te place (as expected) when compiling my v3 code 😝

[patrick-rodgers]

I guess I am not seeing the added value for these proposals over:

interface MyType {
  Title: string;
}

const myVar: MyType = await items.add();

You could say "well this is cleaner" I guess on the below, but what is the real added value?

const myVar = await items.add<MyType>();

[RoelVB]

@patrick-rodgers I don't want to bother you all with this... it was just something on my mind.

I guess it's just personal preference. I would at least return Record<string, any> when we know for sure it's going to return a dictionary. And I really like the T = {Id: number}, because we known it will return at least the Id property. Downside here of course is that we have to define a type when we want more than the Id. I understand from a library perspective this is not a preferable solution.

This was just something, somewhat unrelated to this issue, that came to mind when Julie mentioned the v3 to v4 migration. Don't waste too much time on it, I already appreciate the work you're doing.

[github-actions[bot]]

This issue is locked for inactivity or age. If you have a related issue please open a new issue and reference this one. Closed issues are not tracked.