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

Avoid type assertions in update.ts #6

Merged
merged 3 commits into from Sep 16, 2022

Conversation

grumd
Copy link
Contributor

@grumd grumd commented Sep 14, 2022

Hi.

I've looked at your source code and noticed that you're heavily using type assertions, as well as such code smells as "as any".

I want to use this pull request as an encouragement and example to show that type hacks are unnecessary in your codebase and you can do better. If you want to call your library "type-safe" please make sure your internal code is type-safe first and foremost.

Unfortunately I don't have enough time to fully refactor all of your code, so only took liberty of doing it in a couple of files as a proof of concept.

Cheers.

@netlify
Copy link

netlify bot commented Sep 14, 2022

Deploy Preview for blinkdb canceled.

Name Link
🔨 Latest commit 0aa06fc
🔍 Latest deploy log https://app.netlify.com/sites/blinkdb/deploys/6323a44a62fea60009e5e379

@grumd grumd marked this pull request as draft September 14, 2022 21:07
@grumd
Copy link
Contributor Author

grumd commented Sep 14, 2022

Note: you may want to change your usages of <T> to <T extends object> for the entity type. Doing keyof T when T is not always an object can be troublesome from the type safety standpoint.

@grumd grumd marked this pull request as ready for review September 14, 2022 21:15
let key: keyof T;
for (key in diff) {
const diffValue = diff[key];
if (key === primaryKeyProperty || diffValue === undefined || diffValue === 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.

This is questionable however. What is the expected behaviour when a value in diff is null or undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Expected is that the property value of the item should then be updated with null/undefined.

interface User {
  id: string;
  name: string;
  age?: number;
}

const aliceId = await insert(userTable, { id: uuid(), name: 'Alice', age: 30 });
await update(userTable, { id: aliceId, age: undefined });

const alice = await one(userTable, { id: aliceId });
expect(alice).toStrictEqual({ id: aliceId, name: 'Alice', age: undefined });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For that to work the user will have to have exactOptionalPropertyTypes enabled in their tsconfig (otherwise this happens), but yes, gotcha.

I've updated the pull request to allow for undefined and null when T allows for it. Took some time to do it type-safely though, wasn't easy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, thanks for the link, didn't know that could happen.

@froehlichA
Copy link
Contributor

froehlichA commented Sep 15, 2022

Thanks a lot for your contribution :)

Developing BlinkDB, I wanted to focus on type-safety for the user first. I know that internally, the code isn't as type-safe as it could be, but I hope to remedy that in time.

@@ -2,7 +2,8 @@
"extends": "../../tsconfig.json",
"compilerOptions": {
"outDir": "dist",
"declaration": true
"declaration": true,
"exactOptionalPropertyTypes": true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll create a task on the board about telling users in the docs that they should set this TSconfig option.

@froehlichA froehlichA merged commit bbd64b1 into blinkdb-js:main Sep 16, 2022
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