Skip to content

Conversation

@maparent
Copy link

@maparent maparent commented Dec 23, 2025

Add a templated type to datalog queries.
Major change, requires adding types everywhere, but allows to let go of assertions in most cases.

Summary by CodeRabbit

Release Notes

  • Chores
    • Updated package version to 0.86.0.
    • Refactored internal type system for improved compile-time safety across query operations; no functional changes to user-facing behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

This pull request systematically refactors TypeScript type handling across the codebase by introducing generic type parameters to the Roam API's query method. Instead of explicit type casts, queries now use generic syntax (e.g., q<[string]>() instead of q(...) as string[]). The changes include updating type definitions in src/types/index.ts and src/types/native.ts to support generics, modifying ~40 query files to use the new pattern, converting some imports to type-only imports, and updating the mock environment to propagate generic types. The package version increments to 0.86.0. Functional behavior and public signatures remain unchanged; only type handling mechanisms are refactored.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Templated datalog queries' directly and accurately summarizes the main change: adding generic type parameters to datalog query functions throughout the codebase to enable templated/strongly-typed results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/queries/getFirstChildTextByBlockUid.ts (1)

1-1: Consider updating return type to include undefined.

The function can return undefined when the query finds no results (due to optional chaining on line 4), but the signature declares a string return type. Consider updating to string | undefined for more accurate typing.

🔎 Proposed fix
-const getFirstChildTextByBlockUid = (blockUid: string): string =>
+const getFirstChildTextByBlockUid = (blockUid: string): string | undefined =>
src/queries/getOrderByBlockUid.ts (1)

2-4: Generic type parameter correctly applied.

The addition of <[number]> properly types the query result, improving type safety over the previous explicit cast approach. The optional chaining safely handles cases where the query returns no results.

Optional: Consider making the return type explicit.

The function signature declares number as the return type, but the implementation can return undefined when no block with the given UID exists. While this appears to be pre-existing behavior, you could improve type accuracy by updating the signature to:

const getOrderByBlockUid = (blockUid: string): number | undefined =>

Or provide a default value:

const getOrderByBlockUid = (blockUid: string): number =>
  window.roamAlphaAPI.q<[number]>(
    `[:find ?o :where [?r :block/order ?o] [?r :block/uid "${blockUid}"]]`
  )?.[0]?.[0] ?? 0;
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between be5d99d and c865877.

📒 Files selected for processing (41)
  • package.json
  • src/components/FormDialog.tsx
  • src/queries/getAllBlockUids.ts
  • src/queries/getAllBlockUidsAndTexts.ts
  • src/queries/getAllPageNames.ts
  • src/queries/getAttributeValueByBlockAndName.ts
  • src/queries/getBasicTreeByParentUid.ts
  • src/queries/getBlockUidAndTextIncludingText.ts
  • src/queries/getBlockUidByTextOnPage.ts
  • src/queries/getBlockUidsAndTextsReferencingPage.ts
  • src/queries/getBlockUidsByPageTitle.ts
  • src/queries/getBlockUidsReferencingBlock.ts
  • src/queries/getBlockUidsReferencingPage.ts
  • src/queries/getBlockUidsWithParentUid.ts
  • src/queries/getDisplayNameByEmail.ts
  • src/queries/getDisplayNameByUid.ts
  • src/queries/getEditedUserEmailByBlockUid.ts
  • src/queries/getFirstChildTextByBlockUid.ts
  • src/queries/getLinkedPageTitlesUnderUid.ts
  • src/queries/getNthChildUidByBlockUid.ts
  • src/queries/getOrderByBlockUid.ts
  • src/queries/getPageTitleByBlockUid.ts
  • src/queries/getPageTitleByPageUid.ts
  • src/queries/getPageTitleReferencesByPageTitle.ts
  • src/queries/getPageTitlesAndBlockUidsReferencingPage.ts
  • src/queries/getPageTitlesAndUidsDirectlyReferencingPage.ts
  • src/queries/getPageTitlesReferencingBlockUid.ts
  • src/queries/getPageTitlesStartingWithPrefix.ts
  • src/queries/getPageUidByBlockUid.ts
  • src/queries/getPageViewType.ts
  • src/queries/getParentTextByBlockUid.ts
  • src/queries/getParentTextByBlockUidAndTag.ts
  • src/queries/getParentUidByBlockUid.ts
  • src/queries/getParentUidsOfBlockUid.ts
  • src/queries/getSettingsByEmail.ts
  • src/queries/getShallowTreeByParentUid.ts
  • src/queries/isTagOnPage.ts
  • src/testing/mockRoamEnvironment.ts
  • src/types/index.ts
  • src/types/native.ts
  • src/util/extensionDeprecatedWarning.ts
🧰 Additional context used
🧬 Code graph analysis (18)
src/queries/getBasicTreeByParentUid.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getBlockUidsAndTextsReferencingPage.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getPageTitlesAndUidsDirectlyReferencingPage.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/util/extensionDeprecatedWarning.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getSettingsByEmail.ts (1)
src/types/native.ts (1)
  • UserSettings (430-435)
src/queries/getPageViewType.ts (1)
src/types/native.ts (1)
  • ViewType (303-303)
src/queries/getShallowTreeByParentUid.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getPageTitleReferencesByPageTitle.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getBlockUidsReferencingBlock.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getParentUidByBlockUid.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getAttributeValueByBlockAndName.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getBlockUidsReferencingPage.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/types/index.ts (1)
src/types/native.ts (1)
  • GenericQueryResult (569-569)
src/testing/mockRoamEnvironment.ts (1)
src/types/native.ts (1)
  • GenericQueryResult (569-569)
src/queries/getBlockUidAndTextIncludingText.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getPageTitlesReferencingBlockUid.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getPageTitlesAndBlockUidsReferencingPage.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
src/queries/getParentUidsOfBlockUid.ts (1)
src/types/native.ts (1)
  • PullBlock (238-299)
🔇 Additional comments (45)
src/queries/getParentTextByBlockUid.ts (1)

2-4: LGTM! Type parameter correctly applied.

The generic type parameter <[string]> accurately reflects the datalog query result structure (array of single-element tuples), improving type safety without changing runtime behavior.

src/queries/getParentTextByBlockUidAndTag.ts (1)

8-10: LGTM! Consistent type improvement.

The generic type parameter follows the same pattern as other query refactorings in this PR, correctly typing the datalog query result.

src/queries/getFirstChildTextByBlockUid.ts (1)

2-4: LGTM! Generic typing improves type safety.

The change from explicit type assertion to generic type parameter <[string]> with proper array access is a clear improvement. The query now correctly reflects that it returns an array of tuples, and the [0][0] access extracts the string value properly.

src/queries/getEditedUserEmailByBlockUid.ts (1)

1-4: LGTM! Clean implementation of the new generic type pattern.

The generic type parameter <[string]> correctly represents the query result structure (array of single-element tuples), and the double optional chaining ?.[0]?.[0] appropriately extracts the first email while safely handling empty results. This refactoring improves type safety without altering the function's behavior.

package.json (1)

4-4: LGTM! Appropriate version bump.

The minor version increment from 0.85.7 to 0.86.0 is appropriate for this type system enhancement that introduces generic query typing throughout the codebase.

src/queries/getBlockUidsWithParentUid.ts (1)

3-6: Excellent type safety improvement.

The generic type parameter eliminates the need for the explicit as string cast in the map function. TypeScript now infers the correct type from the query signature.

src/queries/getDisplayNameByEmail.ts (1)

2-4: LGTM!

Generic typing correctly applied. The <[string]> parameter ensures type safety through the optional chaining.

src/queries/getBlockUidsReferencingPage.ts (1)

5-11: LGTM!

The generic type parameter <[PullBlock]> replaces the previous explicit cast, improving type safety. The PullBlock import is correctly retained for the generic type annotation.

src/queries/getPageTitleByPageUid.ts (1)

2-4: LGTM! Cleaner implementation.

The explicit PullBlock cast was unnecessary since .pull() already returns correctly typed results. This simplification improves code clarity.

src/queries/getBlockUidsReferencingBlock.ts (1)

4-8: LGTM!

Consistent application of the generic typing pattern. The <[PullBlock]> type parameter ensures type safety throughout the chain.

src/queries/getPageUidByBlockUid.ts (1)

2-4: LGTM!

The generic type parameter <[{ uid: string }]> correctly describes the query result structure, and the access pattern ?.[0]?.[0]?.uid safely extracts the uid value.

src/types/native.ts (1)

568-569: LGTM! Solid foundation for generic query typing.

The GenericQueryResult type appropriately models Datalog query results as either arrays or objects, enabling the generic .q<T>() pattern throughout the codebase. The use of unknown provides type safety while maintaining flexibility.

src/queries/isTagOnPage.ts (1)

4-10: LGTM! Generic typing improves type safety.

The generic type parameter <[number]> correctly types the query result as a tuple containing a number (entity ID), eliminating the need for runtime assertions while maintaining the boolean existence check logic.

src/queries/getBlockUidsByPageTitle.ts (1)

5-10: LGTM! Clean removal of runtime cast.

The generic type parameter <[string]> provides compile-time type safety, allowing direct access to b[0] without the explicit as string cast. The change correctly maintains the string[] return type.

src/queries/getDisplayNameByUid.ts (1)

2-4: LGTM! Type-safe query with proper fallback.

The generic <[string]> parameter ensures type safety while the optional chaining and empty string default provide robust handling of missing results.

src/queries/getPageTitlesStartingWithPrefix.ts (1)

3-6: LGTM! Consistent generic typing pattern.

The addition of <[string]> and removal of the explicit cast follows the established pattern across the codebase, improving type safety without affecting runtime behavior.

src/queries/getLinkedPageTitlesUnderUid.ts (1)

3-6: LGTM! Type-safe refactor.

The generic <[string]> parameter correctly types the query results, eliminating the need for runtime casting in the map function.

src/queries/getBlockUidByTextOnPage.ts (1)

8-10: LGTM! Type-safe query implementation.

The generic <[string]> ensures proper typing for the query result, with appropriate optional chaining and empty string fallback for robustness.

src/queries/getAllBlockUids.ts (1)

3-4: LGTM! Cleaner type-safe implementation.

The generic type parameter eliminates the runtime cast while maintaining type safety and readability.

src/queries/getSettingsByEmail.ts (1)

4-6: LGTM! Type-safe settings retrieval.

The generic <[UserSettings]> parameter correctly types the query result for a structured object. The type-only import and empty object fallback are appropriate design choices.

src/queries/getParentUidByBlockUid.ts (1)

4-6: LGTM: Generic typing correctly applied.

The change from runtime cast to generic type parameter <[PullBlock]> improves type safety. The nested array access pattern ?.[0]?.[0]?.[":block/uid"] correctly reflects the query result structure: an array of single-element tuples where each tuple contains a PullBlock.

src/components/FormDialog.tsx (2)

28-28: LGTM: Removed unused import.

The PullBlock type is no longer needed after removing the explicit cast at line 177.


174-177: LGTM: Removed unnecessary type cast.

The explicit type cast has been removed while maintaining the same runtime behavior through destructuring. TypeScript will infer the types from the pull result.

src/queries/getBlockUidsAndTextsReferencingPage.ts (1)

7-16: LGTM: Generic typing correctly applied.

The refactor to use .q<[PullBlock]>(...) with array destructuring ([node]) in the map function correctly handles the single-column query result structure. The function signature and behavior remain unchanged.

src/queries/getPageViewType.ts (1)

5-9: LGTM: Generic typing correctly applied.

The change to .q<[ViewType]>(...) with the nested array access ?.[0]?.[0] correctly reflects the query result structure for a single-column datalog query. The fallback to "bullet" is preserved.

src/util/extensionDeprecatedWarning.ts (1)

25-29: LGTM: Generic typing correctly applied.

The refactor to use .q<[PullBlock]>(...) with array destructuring ([block]) in the map function correctly handles the single-column pull query result. The logic remains functionally identical.

src/queries/getBasicTreeByParentUid.ts (1)

15-18: LGTM: Generic typing eliminates runtime cast.

The change to .q<[PullBlock]>(...) removes the need for the explicit as PullBlock cast. The generic type parameter provides compile-time type safety while maintaining the same runtime behavior.

src/queries/getAllBlockUidsAndTexts.ts (1)

3-6: LGTM: Generic typing for multi-column query.

The change to .q<[string, string]>(...) correctly types the two-column datalog query result. The tuple access f[0] and f[1] is now type-safe and correctly reflects the query structure.

src/queries/getShallowTreeByParentUid.ts (1)

6-9: LGTM: Generic typing correctly applied.

The refactor to .q<[PullBlock]>(...) maintains the existing sort and map logic while improving type safety. The access patterns correctly reflect the single-column pull query result structure.

src/queries/getPageTitlesAndUidsDirectlyReferencingPage.ts (1)

7-16: Clean refactor to generic typed query.

The generic q<[PullBlock]> typing correctly represents a result row containing a single PullBlock. The fallback to empty strings properly handles the optional nature of :node/title and :block/uid properties.

src/queries/getBlockUidAndTextIncludingText.ts (1)

6-13: LGTM!

The refactor correctly uses generic typing with appropriate fallbacks for optional PullBlock properties.

src/queries/getAllPageNames.ts (1)

1-4: LGTM!

The q<[string]> generic correctly types the scalar query result. Since the query finds :node/title values directly (not via pull), the result is guaranteed to be strings.

src/queries/getPageTitlesReferencingBlockUid.ts (1)

1-8: LGTM!

Good use of type-only import and optional chaining (s[0]?.) for defensive access to the PullBlock result.

src/queries/getPageTitleReferencesByPageTitle.ts (1)

1-11: LGTM!

The type-only import and generic typing are correct. The direct access p[0][":node/title"] is safe here since the query guarantees a result row structure, and the || "" fallback handles the optional property.

src/queries/getAttributeValueByBlockAndName.ts (1)

1-19: LGTM!

Clean refactor with proper optional chaining for defensive access. The fallback to "" before .slice() is safe since slicing an empty string returns an empty string.

src/queries/getPageTitlesAndBlockUidsReferencingPage.ts (1)

1-16: LGTM!

The q<[PullBlock, PullBlock]> generic correctly represents the two-pull query structure. The type-only import and fallback handling are appropriate.

src/types/index.ts (2)

3-3: LGTM!

The import of GenericQueryResult is necessary for the new generic type constraints and properly imported from the native types module.


85-88: LGTM! Well-structured generic type signatures.

The addition of generic type parameters to all query methods is consistent and well-designed:

  • Properly constrained with extends GenericQueryResult
  • Return types correctly use T[] for synchronous methods and Promise<T[]> for asynchronous methods
  • Maintains backward compatibility while enabling strong typing at call sites

Also applies to: 120-123, 126-129, 139-142, 146-149, 161-164

src/testing/mockRoamEnvironment.ts (4)

6-6: LGTM!

The import of GenericQueryResult is correctly added to support the new generic function signatures.


115-126: Type assertion in test mock - caller responsibility.

The generic signature and type assertion as T[] are appropriate for this mock implementation. Ensure test code specifies the correct generic type parameter when calling this function, as there's no runtime validation of the result shape.

Also applies to: 1186-1186


1189-1195: LGTM!

The generic type parameter is properly threaded through mockQuery to fireQuery<T>, maintaining type safety across the mock implementation.

Also applies to: 1380-1380


1412-1416: LGTM!

The mock q method correctly implements the new generic signature, properly propagating the type parameter to mockQuery<T>.

src/queries/getParentUidsOfBlockUid.ts (1)

4-8: Improved type safety with generic type parameter.

The refactoring to use method chaining with <[PullBlock]> generic type parameter improves compile-time type safety and eliminates the need for runtime type assertions. The generic type correctly represents the datalog query return type (array of tuples, each containing one PullBlock), and the defensive || "" fallback appropriately handles the optional :block/uid property.

src/queries/getNthChildUidByBlockUid.ts (2)

10-12: LGTM! Type refactor is correct.

The generic type parameter <[PullBlock]> accurately describes the query result structure (array of tuples, each containing one PullBlock), and the chaining ?.[0]?.[0]?.[":block/uid"] correctly extracts the UID from the nested result. This cleanly eliminates the need for runtime type assertions.


10-12: No action required. The blockUid parameter originates exclusively from Roam's internal DOM elements (data-uid attributes and element IDs extracted via getUidsFromId), not from user input. Since Roam is a closed-source system that controls these values, injection risk is mitigated by design. This interpolation pattern is standard throughout the codebase for system-generated UIDs.

Comment on lines +2 to +4
window.roamAlphaAPI.q<[{ title: string }]>(
`[:find (pull ?p [:node/title]) :where [?e :block/uid "${blockUid}"] [?e :block/page ?p]]`
)?.[0]?.[0]?.title || "";
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Type mismatch: pull result uses :node/title, not title.

The Roam pull expression (pull ?p [:node/title]) returns { ":node/title": string }, but the generic type specifies { title: string }. The access .title will always be undefined, causing the function to always return "".

🔎 Proposed fix
 const getPageTitleByBlockUid = (blockUid: string): string =>
-  window.roamAlphaAPI.q<[{ title: string }]>(
+  window.roamAlphaAPI.q<[{ ":node/title": string }]>(
     `[:find (pull ?p [:node/title]) :where [?e :block/uid "${blockUid}"] [?e :block/page ?p]]`
-  )?.[0]?.[0]?.title || "";
+  )?.[0]?.[0]?.[":node/title"] || "";

Alternatively, use PullBlock for consistency with other query files:

+import type { PullBlock } from "../types";
+
 const getPageTitleByBlockUid = (blockUid: string): string =>
-  window.roamAlphaAPI.q<[{ title: string }]>(
+  window.roamAlphaAPI.q<[PullBlock]>(
     `[:find (pull ?p [:node/title]) :where [?e :block/uid "${blockUid}"] [?e :block/page ?p]]`
-  )?.[0]?.[0]?.title || "";
+  )?.[0]?.[0]?.[":node/title"] || "";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
window.roamAlphaAPI.q<[{ title: string }]>(
`[:find (pull ?p [:node/title]) :where [?e :block/uid "${blockUid}"] [?e :block/page ?p]]`
)?.[0]?.[0]?.title || "";
window.roamAlphaAPI.q<[{ ":node/title": string }]>(
`[:find (pull ?p [:node/title]) :where [?e :block/uid "${blockUid}"] [?e :block/page ?p]]`
)?.[0]?.[0]?.[":node/title"] || "";
🤖 Prompt for AI Agents
In src/queries/getPageTitleByBlockUid.ts around lines 2 to 4, the Roam pull
returns an object with key ":node/title" but the code types and accesses it as
`title`, causing undefined; update the TypeScript generic to match the actual
pull shape (e.g. `{ ":node/title": string }`) and read that property, or replace
the generic with the existing `PullBlock` type used elsewhere and access the
correct field from the pulled object before returning the string; ensure the
fallback remains "" if the value is missing.

Copy link
Collaborator

@mdroidian mdroidian left a comment

Choose a reason for hiding this comment

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

While there is a small DX benefit to typing query results at the call site, this PR introduces a repo-wide breaking change with little to no end-user value, and it surfaces new type-contract issues. I can’t take on a migration and re-testing effort across 10+ extensions right now, so this needs to be re-scoped to an opt-in, non-breaking approach.

Key concerns

1) This is still an unchecked assertion (not real type safety)

Moving from as string / as PullBlock to .q<[string]>(...) improves inference, but it is still an unchecked claim about runtime shape. In several places this risks giving a stronger appearance of safety without adding any actual guarantees.

2) GenericQueryResult = unknown[] | Record<string, unknown> is too permissive as a default

Most Roam datalog queries return tuple rows (unknown[]). Record/object rows are primarily produced by :keys (and a small subset of patterns). Making the default API accept both shapes allows incorrect typing with no signal at the call site and reduces clarity about when record semantics apply.

3) Return type contracts remain incorrect

Several helpers still return possibly-undefined values but are typed as string / number (e.g. getFirstChildTextByBlockUid, getOrderByBlockUid). Previously this mismatch was visible via explicit casts; this PR replaces those casts with generic typing, which can read as safer while remaining unchecked.

Request: either add safe defaults (|| "", || 0) or update the function signatures to reflect undefined.

4) Migration cost is high relative to value

This change touches a large surface area and would force downstream consumers to update patterns and potentially re-test queries. Given current priorities and existing work, I don’t believe we should take this on at this time, especially for a change that doesn’t materially improve functionality.

Recommendation / path forward

If you decide to continue exploring this direction (which I would not recommend at this time), I’d suggest the following instead:

A) Re-scope to a non-breaking, opt-in approach

Rather than changing global q typings and migrating existing call sites:

  1. Preserve the current q behavior (or at least avoid forcing migration).

  2. Introduce explicit helper APIs, for example:

    • qTuple<T extends readonly unknown[]>(query, ...params): T[]
    • qKeys<T extends Readonly<Record<string, unknown>>>(query, ...params): T[] (explicitly for :keys)
  3. Add documentation and examples clarifying when record rows (:keys) apply versus tuple rows (default).

B) Adopt gradually

With helpers in place, we can:

  • use them in new code only
  • optionally migrate touched code over time
  • avoid forcing updates across all existing extensions

};
};

export type GenericQueryResult = unknown[] | Record<string, unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For something like this, examples of how/when Record<string, unknown> applies would help document this rarely used case.


roamAlphaAPI: {
q: (query: string, ...params: unknown[]) => unknown[][];
q: <T extends GenericQueryResult>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

While queries using :keys return record/object rows, most queries do not. Therefore, I believe q<T extends unknown[] | Record<string, unknown>> is too permissive. I'd prefer separate APIs or an explicit opt-in for keyed queries.


const getFirstChildTextByBlockUid = (blockUid: string): string =>
window.roamAlphaAPI.q(
window.roamAlphaAPI.q<[string]>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can return undefined.

const getOrderByBlockUid = (blockUid: string): number =>
window.roamAlphaAPI.q(
window.roamAlphaAPI.q<[number]>(
`[:find ?o :where [?r :block/order ?o] [?r :block/uid "${blockUid}"]]`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can return undefined.

@maparent
Copy link
Author

First, thanks for taking the time to comment.

I agree that making this a breaking change is probably too aggressive on downstream users; I went ahead to ascertain how much effort it was to do this with an existing codebase.

You propose adding helper functions; I am totally in favour, but I'm less sure about implementation. There are six instances of the q method in the roam api; we could create 12 helper functions that live in their own namespace within roamjs (window.roamjs.qTuple, widow.roamjs.data.async.fast.qRecord etc.), or put the helper functions alongside each q function by altering the module, which is probably more intuitive but feels a bit hacky.
(i.e. window.roamAlphaAPI.qTuple = window.roamAlphaAPI.q etc.)
Or do you have another proposal?

As for priority... this initiative very much stems from the conversation we had about avoiding or justifying assertions:
DiscourseGraphs/discourse-graph#593 (comment)
(I recall you even thought of making this into a linting rule, I think verbally.)
I am absolutely of the advice that making justification of assertions into a requirement would add a truly annoying amount of useless boilerplate in this precise case, and would insist that the change I propose should come before the requirement to justify assertions, whenever that comes about.

You proposed I should make this into a Roam issue, I am happy to but propose we should make another round of clarifications here before I do so. (And I think the decision to add constraints to commits should also be subject to a similar group cost/benefit analysis.)

Side note: re adding unknown to the signature vs adding an empty default: I think this one requires some judgement, not sure I can get it right on my own. It's certainly the case that making the change on q itself made the issue more explicit, I made at least one change accordingly. I see this as a type safety benefit of this initiative.

@mdroidian
Copy link
Collaborator

Or do you have another proposal?

At this moment, I do not. A change of this magnitude would take some time to consider the options. Unfortunately, I have some other very pressing matters that are taking priority.

As for priority ... linting rule ..

Rest assured there will be no linting rule until this case is taken care of. 👍

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