Pnpjs: Can't make batching work in sp-taxonomy

Created on 14 Dec 2018  路  12Comments  路  Source: pnp/pnpjs

Category

  • [ ] Enhancement
  • [x] Bug
  • [x] Question
  • [ ] Documentation gap/issue

Version

Please specify what version of the library you are using: [1.2.7]

Please specify what version(s) of SharePoint you are targeting: [SharePoint Online]

Expected / Desired Behavior / Question

Batching for sp-taxonomy works

Observed Behavior

I don't see any outgoing requests after batch.execute

Steps to Reproduce

    taxonomy.setup({
      spfxContext: this.context
    });

    var termStores = await taxonomy.termStores.get();
    let returnTerms: IPickerTerm[] = [];

    for (let i = 0, len = termStores.length; i < len; i++) {
      const pnpTermStore = termStores[i];
      const pnpTerms = await pnpTermStore.getTerms({
        TermLabel: 'Mar',
        StringMatchOption: StringMatchOption.StartsWith,
        DefaultLabelOnly: true,
        TrimUnavailable: true,
        ResultCollectionSize: 10
      }).get();

      const batch = taxonomy.createBatch();
      // in real scenario there should be pnpTerms.forEach
      const term = pnpTerms[0];

      term.termSet.inBatch(batch).get().then(termSet => {
        console.log(termSet);        
      });

      term.labels.inBatch(batch).get().then(labels => {
        console.log(labels);
      });

      await batch.execute();
code fixed bug

Most helpful comment

This turned out to be a great bug because it forced me to revisit how the batches are blocked from executing too soon (which was the root issue here). The new method is much simpler and does not rely on any timing, we now create the dependency immediately when the batch is first added. Much simpler. Will close this with the merge of #432.

All 12 comments

I believe the await on execute is blocking. Because you used await and execute returns a promise you are hanging the application there and the code to run the requests can't run. Please try switching that to a .then and things should work OK.

Tried .then - no luck.

Full code of my SPFx onInit function is below. Requests that are not batched work fine.
Batched ones are not being sent:

public async onInit(): Promise<void> {
    taxonomy.setup({
      spfxContext: this.context
    });

    var termStores = await taxonomy.termStores.get();
    let returnTerms: IPickerTerm[] = [];

    for (let i = 0, len = termStores.length; i < len; i++) {
      const pnpTermStore = termStores[i];
      const pnpTerms = await pnpTermStore.getTerms({
        TermLabel: 'Mar',
        StringMatchOption: StringMatchOption.StartsWith,
        DefaultLabelOnly: true,
        TrimUnavailable: true,
        ResultCollectionSize: 10
      }).get();

      const batch = taxonomy.createBatch();

      pnpTerms.forEach(term => {
        const pickerTerm: IPickerTerm = {
          key: term.Id,
          name: term.Name,
          path: term.PathOfTerm,
          termSet: ''
        };
        returnTerms.push(pickerTerm);

        term.termSet.inBatch(batch).get().then(termSet => {
          pickerTerm.termSet = termSet.Id;
          pickerTerm.termSetName = termSet.Name;
        });

        term.labels.inBatch(batch).get().then(labels => {
          pickerTerm.labels = labels.map(label => label.Value);
        });
      });

      batch.execute().then(() => { console.log(returnTerms); });
    }
    return super.onInit();
  }

Yep, just checked and something is indeed broken. Appears to be some type of timing issue where we aren't waiting for something properly. Not sure but will update thread once I know more. Thanks for letting us know, we'll have a look.

This turned out to be a great bug because it forced me to revisit how the batches are blocked from executing too soon (which was the root issue here). The new method is much simpler and does not rely on any timing, we now create the dependency immediately when the batch is first added. Much simpler. Will close this with the merge of #432.

Great! Glad to help in improving! Indirectly :)

Still have issues with batches in in 1.2.8-1.
Now I see the outgoing request but I'm getting Invalid Request. error.
I tried multiple scenarios - loading multiple terms, multiple term sets or multiple properties for single term (code is below). All of them are errored.
Then I tried to use batch with a single request inside - and it works.
So, it looks like there is some issue when there are multiple requests in the batch.
I can create a separate issue if needed
Here is an example of the code:

    taxonomy.setup({
      spfxContext: this.context
    });

    const termStore = await taxonomy.getDefaultSiteCollectionTermStore().get();

    const terms = await termStore.getTerms({
      TermLabel: 'Dev',
      StringMatchOption: StringMatchOption.StartsWith,
      DefaultLabelOnly: true,
      TrimUnavailable: true,
      ResultCollectionSize: 10
    }).get();

    console.log(terms);

    const batch = taxonomy.createBatch();

    terms.forEach(term => {
      term.termSet.inBatch(batch).get().then(termSet => {
        console.log(termSet.Id);
      });
      term.labels.inBatch(batch).get().then(labels => {
        console.log(labels);
      });
    });

    batch.execute().then(() => { 
      console.log('executed'); 
    }, (error) => { 
      console.log(error); 
    });

Did you update all of the libraries to the same beta version?

yup...
beta

The issue here is actually really complicated. The below is a work around for your case:

terms.forEach(term => { 

    taxonomy.getDefaultSiteCollectionTermStore().getTermById(term.Id).termSet.inBatch(batch).get().then(termSet => {
            console.log(termSet.Id);
        });

    taxonomy.getDefaultSiteCollectionTermStore().getTermById(term.Id).labels.inBatch(batch).get().then(labels => {
            console.log(labels);
        });

});

The problem is that if you look at the xml body of the request being sent you will see that Id's are duplicated within the request body. That's isn't allowed (for obvious reasons) BUT it isn't a quick fix as I think it might take some larger changes to the internals. Or maybe not, but I did a few quick things and it broke a lot of stuff.

Thanks @patrick-rodgers for the work around.
I'll be using it for now and let you know if I find any other helpful info regarding this one.

Thanks again!

I just merged a fix for this and published a new beta. Can you please give that a try with your original code. It now works for me as you had it in this thread at the start. If you are still having issues please open a new thread and ref this one. Thanks!

Yay! It's working now!
You're awesome @patrick-rodgers!

Was this page helpful?
0 / 5 - 0 ratings