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

Fix generated write typings when WebResource are included #53

Closed
wants to merge 1 commit into from

Conversation

Page-
Copy link
Contributor

@Page- Page- commented Mar 12, 2024

Change-type: patch

@Page- Page- requested review from otaviojacobi and a team March 12, 2024 11:35
@flowzone-app flowzone-app bot enabled auto-merge March 12, 2024 11:39
@otaviojacobi
Copy link
Contributor

@Page- Why is this necessary and not the other typings? This WebResource typing should ever only be read as this (and the writing is done by uploading a file)

@Page-
Copy link
Contributor Author

Page- commented Mar 12, 2024

@otaviojacobi the write typings refer to the WebResource interface, I was just adding the import so that the generated typings can be valid. If the correct typing is for them to not use that interface and instead use something else then feel free to PR that instead

@Page-
Copy link
Contributor Author

Page- commented Mar 12, 2024

You can check the test at https://github.com/balena-io-modules/abstract-sql-to-typescript/blob/v2.1.4/test/index.ts which has the write typings referring to a WebResource type which doesn't exist in that context

@otaviojacobi
Copy link
Contributor

otaviojacobi commented Mar 12, 2024

I see. This is a bit messy right now then, as what currently happens is that the SDK replaces the WebResource typing on writes. I guess the correct thing to do here is to have the proper write type as something like

interface WebResourceWrite extends Blob {
    name: string;
}

For the write typings, which requires a major for node >= 18 or just leave as is, the problem is we must always remember to replace the correct write types on the client post/patches

@otaviojacobi
Copy link
Contributor

Actually the SDK does not use the abstract-sql-to-typescript thing, it is manually mantained, so it doesn't really matter, I will PR a major with the correct write typings. The problematic part is that pinejs also supports node 16, so it will probably require a major in there aswell....

@otaviojacobi
Copy link
Contributor

@Page- See #54

@Page- Page- marked this pull request as draft March 12, 2024 13:27
auto-merge was automatically disabled March 12, 2024 13:27

Pull request was converted to draft

@Page- Page- closed this May 30, 2024
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