[web-db] add `sql.js` types
ClosedPublic
Actions

sql.js is just a normal npm package correct? Can you create a stub with flow-typed create-stub sql.js and replace the generated types with your code (keep the flow-typed signature and flow-typed version comments), so we are consistent with the rest of the types
You typed everything as a global and so I don't think the initSqlJs function is actually typed in db-worker.js (the global initSqlJs is overridden by the imported one which is still typed as any). You can type this package as a module?

web/flow-typed/SQLiteJS.js
4 ↗	(On Diff #23869)	Does this need to be inexact?
20–21 ↗	(On Diff #23869)	Typescript uses `typeof` keyword for typing these fields. I think they are supposed to hold classes themselves and not instances so we will also need to use `typeof` but please correct me if I'm wrong.
25 ↗	(On Diff #23869)	Can we type the `config` better? In the sql.js documentation, there's `locateFile` specified (and in the emscripten documentation there are even more fields but we probably don't need to add the all)
66 ↗	(On Diff #23869)	In typescript, it's typed as both `Iterable` and `Iterator`. Can we do the same?

This revision now requires changes to proceed.Mar 22 2023, 4:45 AM

kamil mentioned this in D7118: [web-db] add function for parsing SQLite query.Mar 22 2023, 5:32 AM

kamil added a child revision: D7118: [web-db] add function for parsing SQLite query.

ashoat requested changes to this revision.Mar 22 2023, 2:48 PM

ashoat added inline comments.

web/flow-typed/SQLiteJS.js
1 ↗	(On Diff #23869)	You are taking a very non-standard approach here. Can you please make this file look like the rest, instead of polluting the global namespace with type declarations? Start with `flow-typed stub`. Pay attention to where the file is created and what the naming convention is. Note that all declarations are wrapped in a `declare module`.

address code review

kamil added inline comments.Mar 23 2023, 5:10 AM

web/database/worker/db-worker.js
51	Because it was badly typed previously I needed to do some enhancement here to satisfy Flow. It is refactored later in the stack anyway
web/flow-typed/SQLiteJS.js
1 ↗	(On Diff #23869)	Yeah, I did an awful job here, I put up this diff too chaotically without checking what will be best. Creating stub and wrapping everything in declare module as you suggested
4 ↗	(On Diff #23869)	Hmm... that's how it is typed in typescript, but inexact it's not needed in a way we will use it so I'll update this to exact for now.
20–21 ↗	(On Diff #23869)	good catch!
25 ↗	(On Diff #23869)	Making this inexact type with `locateFile` funciton
66 ↗	(On Diff #23869)	I'm not sure if there is a need for this. Also there will be a problem because for `Iterable` the `next()` function is typed as: { +done: boolean, +value: ..., }; while for `Iterator` it is: \| { +done: true, +value: ..., } \| { +done: false, +value: ..., };

Harbormaster completed remote builds in B17543: Diff 24017.Mar 23 2023, 5:20 AM

ashoat accepted this revision.Mar 23 2023, 12:37 PM

ashoat added inline comments.

web/flow-typed/npm/sql.js_vx.x.x.js
17–18	You set these properties to be read-only, but the values are not. In general we usually want read-only values if we're using a read-only property On the other hand, if this is only used as a return type, and we don't care about passing it around, AND we know that all of the objects returned here are created fresh every single time, then we could make this type non-readonly
27–28	I think you can also use `Class<>` instead of `typeof`, which is slightly more Flow-like but it honestly doesn't really matter
44	Why is this `create_function` instead of `createFunction`? I guess it's sql.js being inconsistent with naming?

ashoat added inline comments.Mar 23 2023, 12:54 PM

web/flow-typed/npm/sql.js_vx.x.x.js
59	Why is this synchronous? I would've expected an async call that returns a `Promise`, but I guess the call basically just calls the underlying SQLite function, and it's all in a web worker so it's okay?

address review

web/flow-typed/npm/sql.js_vx.x.x.js
17–18	According to this comment: https://phab.comm.dev/D7118?id=23917#inline-46618 I wanted to make this non-read-only, I just forgot to update properties. Objects are created fresh each time so I will make then non-readonly
44	Yeah, that's the case, example inconsistency: create_function getRowsModified
59	`sql.js` API is synchronous because it’s based on the C API (basically SQLite engine compiled to WebAssymbly). Only worker uses this API and I've implemented app<->worker connection using Promises so it will not be a problem

Harbormaster completed remote builds in B17589: Diff 24078.Mar 24 2023, 8:19 AM

michal accepted this revision.Mar 24 2023, 9:29 AM

This revision is now accepted and ready to land.Mar 24 2023, 9:29 AM

Thanks for explaining!

reabse before landing

Harbormaster completed remote builds in B17617: Diff 24111.Mar 25 2023, 3:49 AM

Closed by commit rCOMM3f1a02bb68b0: [web-db] add `sql.js` types (authored by kamil). · Explain WhyMar 25 2023, 4:02 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM3f1a02bb68b0: [web-db] add `sql.js` types.

Revision Contents
Changeset List

Path

Size

web/

database/

worker/

db-worker.js

12 lines

flow-typed/

npm/

sql.js_vx.x.x.js

89 lines

Diff 24017

View Options

web/database/worker/db-worker.js

// @flow		// @flow

		import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native';		import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant';		import invariant from 'invariant';
import * as React from 'react';		import * as React from 'react';
import { View } from 'react-native';		import { View, Text } from 'react-native';

import genesis from 'lib/facts/genesis.js';		import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';		import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import {		import {
userInfoSelectorForPotentialMembers,		userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers,		userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js';		} from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js';		import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines	if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights;		this.pendingListDataWithHeights = listDataWithHeights;
} else {		} else {
this.setState({ listDataWithHeights });		this.setState({ listDataWithHeights });
}		}
};		};
}		}

const unboundStyles = {		const unboundStyles = {
		pinnedCountBanner: {
		backgroundColor: 'panelForeground',
		height: 30,
		flexDirection: 'row',
		textAlign: 'center',
		justifyContent: 'center',
		alignItems: 'center',
		},
		pinnedCountText: {
		color: 'panelBackgroundLabel',
		marginRight: 5,
		},
container: {		container: {
backgroundColor: 'listBackground',		backgroundColor: 'listBackground',
flex: 1,		flex: 1,
},		},
threadContent: {		threadContent: {
flex: 1,		flex: 1,
},		},
hiddenThreadContent: {		hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines	) {
const styles = useStyles(unboundStyles);		const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext);		const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer();		const measureMessages = useHeightMeasurer();

const genesisThreadInfo = useSelector(		const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id],		state => threadInfoSelector(state)[genesis.id],
);		);

		const bannerText = React.useMemo(() => {
		if (!threadInfo.pinnedCount \|\| threadInfo.pinnedCount === 0) {
		return '';
		}

		const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
		return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
		}, [threadInfo.pinnedCount]);
		atulUnsubmitted Not Done Inline Actions You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (`==` in JS) and we can avoid re-renders. This is helpful for objects (including `Map()`, `Set()`, etc), arrays (which are objects), and functions (which are objects). On the other hand, `string`s in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same. See below: (In like C++, which you've been working w/ recently, `std::string`s can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?) atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…

		const pinnedCountBanner = React.useMemo(() => {
		if (!bannerText) {
		return null;
		}

		return (
		<View style={styles.pinnedCountBanner}>
		<Text style={styles.pinnedCountText}>{bannerText}</Text>
		<Icon
		name="chevron-right"
		size={12}
		color={colors.panelBackgroundLabel}
		/>
		</View>
		);
		}, [
		bannerText,
		styles.pinnedCountBanner,
		styles.pinnedCountText,
		colors.panelBackgroundLabel,
		]);

return (		return (
<MessageListContextProvider threadInfo={threadInfo}>		<MessageListContextProvider threadInfo={threadInfo}>
		{pinnedCountBanner}
<MessageListContainer		<MessageListContainer
{...props}		{...props}
usernameInputText={usernameInputText}		usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput}		updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray}		userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput}		updateTagInput={updateTagInput}
resolveToUser={resolveToUser}		resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos}		otherUserInfos={otherUserInfos}
Show All 14 Lines

View Options

web/flow-typed/npm/sql.js_vx.x.x.js

// @flow		// @flow

		import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native';		import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant';		import invariant from 'invariant';
import * as React from 'react';		import * as React from 'react';
import { View } from 'react-native';		import { View, Text } from 'react-native';

import genesis from 'lib/facts/genesis.js';		import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';		import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import {		import {
userInfoSelectorForPotentialMembers,		userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers,		userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js';		} from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js';		import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines	if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights;		this.pendingListDataWithHeights = listDataWithHeights;
} else {		} else {
this.setState({ listDataWithHeights });		this.setState({ listDataWithHeights });
}		}
};		};
}		}

const unboundStyles = {		const unboundStyles = {
		pinnedCountBanner: {
		backgroundColor: 'panelForeground',
		height: 30,
		flexDirection: 'row',
		textAlign: 'center',
		justifyContent: 'center',
		alignItems: 'center',
		},
		pinnedCountText: {
		color: 'panelBackgroundLabel',
		marginRight: 5,
		},
container: {		container: {
backgroundColor: 'listBackground',		backgroundColor: 'listBackground',
flex: 1,		flex: 1,
},		},
threadContent: {		threadContent: {
flex: 1,		flex: 1,
},		},
hiddenThreadContent: {		hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines	) {
const styles = useStyles(unboundStyles);		const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext);		const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer();		const measureMessages = useHeightMeasurer();

const genesisThreadInfo = useSelector(		const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id],		state => threadInfoSelector(state)[genesis.id],
);		);

		const bannerText = React.useMemo(() => {
		if (!threadInfo.pinnedCount \|\| threadInfo.pinnedCount === 0) {
		return '';
		}

		const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
		return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
		}, [threadInfo.pinnedCount]);
		atulUnsubmitted Not Done Inline Actions You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (`==` in JS) and we can avoid re-renders. This is helpful for objects (including `Map()`, `Set()`, etc), arrays (which are objects), and functions (which are objects). On the other hand, `string`s in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same. See below: (In like C++, which you've been working w/ recently, `std::string`s can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?) atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…

		const pinnedCountBanner = React.useMemo(() => {
		if (!bannerText) {
		return null;
		}

		return (
		<View style={styles.pinnedCountBanner}>
		<Text style={styles.pinnedCountText}>{bannerText}</Text>
		<Icon
		name="chevron-right"
		size={12}
		color={colors.panelBackgroundLabel}
		/>
		</View>
		);
		}, [
		bannerText,
		styles.pinnedCountBanner,
		styles.pinnedCountText,
		colors.panelBackgroundLabel,
		]);

return (		return (
<MessageListContextProvider threadInfo={threadInfo}>		<MessageListContextProvider threadInfo={threadInfo}>
		{pinnedCountBanner}
<MessageListContainer		<MessageListContainer
{...props}		{...props}
usernameInputText={usernameInputText}		usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput}		updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray}		userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput}		updateTagInput={updateTagInput}
resolveToUser={resolveToUser}		resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos}		otherUserInfos={otherUserInfos}
Show All 14 Lines

[web-db] add `sql.js` typesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 24017

web/database/worker/db-worker.js

web/flow-typed/npm/sql.js_vx.x.x.js

[web-db] add `sql.js` types
ClosedPublic
Actions

Revision Contents
Changeset List