Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Define types for performBulk #786

Merged
merged 4 commits into from
May 15, 2024
Merged

feat(core): Define types for performBulk #786

merged 4 commits into from
May 15, 2024

Conversation

kola-er
Copy link
Contributor

@kola-er kola-er commented May 14, 2024

No description provided.

Comment on lines 20 to 59
test('create movies in bulk', async () => {
const genre = 'Genre Test';
const idem1 = 'test-id-xxx';
const idem2 = 'test-id-yyy';
const idem3 = 'test-id-zzz';

const bulkBundle = {
bulk: [
{
inputData: { genre, title: 'Title 1', year: 2020 },
meta: { id: idem1 },
},
{
inputData: { genre, title: 'Title 2', year: 2021 },
meta: { id: idem2 },
},
{
inputData: { genre, title: 'Title 3', year: 2022 },
meta: { id: idem3 },
},
],
groupedBy: { genre },
};

const result = await appTester(App.creates.movies.operation.performBulk, bulkBundle);
expect(result).toMatchObject({
idem1: {
outputData: { genre, id: '1', title: 'Title 1', year: 2020 },
error: null,
},
idem2: {
outputData: { genre, id: '2', title: 'Title 2', year: 2021 },
error: null,
},
idem3: {
outputData: { genre, id: '3', title: 'Title 3', year: 2022 },
error: null,
},
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eliangcs Is this the test you referred to in the ticket or a manual one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the Bundle and the new BulkBundle are mixed up in the failing tests in this file. I'm confused about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kola-er sorry for the confusion! A manual test should be enough: Manually create a TypeScript integration project, add a create with performBulk, and use the new index.d.ts to see it's working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the new index.d.ts with this example:

import {
  ZObject,
  BulkBundle,
  PerformBulkResult,
} from 'zapier-platform-core';

const performBulk = async (
  z: ZObject,
  bundle: BulkBundle<{ title: string; year: number }>
): Promise<PerformBulkResult> => {
  const movies = bundle.bulk.map((b) => {
    return {
      _idempotencyKey: b.meta.id,
      title: b.inputData.title,
      year: b.inputData.year,
    };
  });
  const response = await z.request({
    method: 'POST',
    url: 'https://example.com/api/movies/',
    body: {
      objects: movies,
    },
  });
  return movies.reduce(
    (result: PerformBulkResult, movie: { [x: string]: any }) => {
      result[movie._idempotencyKey] = {
        outputData: {
          title: movie.title,
          year: movie.year,
        },
      };
      return result;
    },
    {}
  );
};

export default {
  key: 'movie',
  noun: 'Movie',

  display: {
    label: 'Create Movie',
    description: 'Creates a new movie.',
  },

  operation: {
    performBulk,
    inputFields: [
      { key: 'title', required: true },
      { key: 'year', type: 'integer' },
    ],
    sample: {
      id: '1',
      title: 'example',
    },
  },
};

It works great! It was able to detect type issues. For example, when I replaced outputData with outputData2, my IDE would show:

'outputData2' does not exist in type 'PerformBulkResult`

which is good 👍

Remove the new code you added in example-apps/typescript and then I think this is good to merge. Thanks!

Comment on lines 216 to 219
export interface PerformBulkItem {
outputData: { [x: string]: any };
error?: string;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with #785, only one of outputData or error is required. Maybe we could do this with two interfaces?

export interface PerformBulkSuccessItem {
  outputData: { [x: string]: any };
}

export interface PreformBulkErrorItem {
  error: string;
}

export type PerformBulkItem = PerformBulkSuccessItem | PerformBulkErrorItem;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are the possible cases (outputData and error) or (outputData) or (error) for the and/or ;

export interface PerformBulkSuccessItem {
  outputData: { [x: string]: any };
  error?: string;
}

export interface PreformBulkErrorItem {
  outputData?: { [x: string]: any };
  error: string;
}

export type PerformBulkItem = PerformBulkSuccessItem | PerformBulkErrorItem;

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So outputData and error can coexist. That makes sense!

@kola-er kola-er merged commit e88b4e5 into release-15.8.0 May 15, 2024
13 checks passed
@kola-er kola-er deleted the PDE-4991 branch May 15, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants