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

Update pinejs-client and make use of improved typings #54

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

Page-
Copy link
Contributor

@Page- Page- commented Jul 15, 2024

Change-type: patch

@Page- Page- requested a review from a team July 15, 2024 11:43
@Page- Page- enabled auto-merge July 15, 2024 11:43
.get({
resource: 'user',
id,
options: { $select: 'id' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This $select was previously missing

.get({
resource: 'application',
id,
options: { $select: 'id' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This $select was previously missing

options: { $select: 'id' },
} as const)
.catch(models.wrapResponseError);
if (user == 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.

It could always be null but in the past it would have error'ed with something like TypeError: cannot read 'id' of undefined downstream

options: { $select: 'id' },
} as const)
.catch(models.wrapResponseError);
if (app == 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.

It could always be null but in the past it would have error'ed with something like TypeError: cannot read 'id' of undefined downstream

}

async function getOrCreateService(
api: PinejsClientCore<unknown>,
body: models.ServiceAttributes,
): Promise<models.ServiceModel> {
return models.getOrCreate(api, 'service', body, {
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 replaced this with the built in getOrCreate to keep things standardized with other repos using the client

Copy link
Contributor

@thgreasi thgreasi left a comment

Choose a reason for hiding this comment

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

Glad we got rid of the extra models.ts wrapper, since pinejs-client is our sdk for OData and we shouldn't need an extra sdk on top of it for such simple operations.

package.json Outdated
Comment on lines 86 to 87
"pinejs-client-core": "^6.15.10",
"pinejs-client-request": "^7.5.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the sdk is pinned to ~6.14.0, bumping these would cause headaches in the CLI by causing it to have to separate versions of pinejs-client-core. Would it be a big deal deferring this bump for a bit (until the next sdk major)?

@Page- Page- force-pushed the update-pinejs-client branch from 6ad2ec9 to 01937a9 Compare July 15, 2024 15:36
@Page- Page- marked this pull request as draft July 15, 2024 15:37
auto-merge was automatically disabled July 15, 2024 15:37

Pull request was converted to draft

@Page- Page- force-pushed the update-pinejs-client branch from 01937a9 to 60117af Compare July 15, 2024 15:41
@Page- Page- marked this pull request as ready for review July 15, 2024 15:41
@Page- Page- enabled auto-merge July 15, 2024 15:41
@Page- Page- force-pushed the update-pinejs-client branch 2 times, most recently from 8b71424 to 1085c2c Compare July 19, 2024 12:24
@Page- Page- force-pushed the update-pinejs-client branch from 1085c2c to 055a907 Compare July 19, 2024 12:27
@Page- Page- merged commit dfe8b09 into master Jul 19, 2024
50 checks passed
@Page- Page- deleted the update-pinejs-client branch July 19, 2024 12:33
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.

2 participants