new arg to filter by subclass ref #2105

tintinthong · 2025-01-30T08:49:34Z

Purpose of this PR is to enable spec to choose linkedExamples that link by type

API change

<@fields @subclassType={{ref}} />. This means users can control the linksTo and linksToMany editor via this arg. It will only work if you pass in a ref which is indeed a subclass of the type. Otherwise, it defaults to original behaviour
I named it subclassType as the arg @lukemelia different from here https://discord.com/channels/584043165066199050/900021750367129641/1334556182001483826 bcos, I think its wrong to overwrite the field entirely. Now the api works st, you only pass the ref that way @fields.author is still true to the type of author but you can for example, <@fields.author @subclassType=bigAuthorRef>. The issue with adding another filter to the every = [ ] in the card chooser is that the index never supports 2 types so we always have to commit to one

github-actions · 2025-01-30T09:03:19Z

Host Test Results

    1 files ±0     1 suites ±0 23m 17s ⏱️ -50s
765 tests +1 763 ✔️ +1 2 💤 ±0 0 ❌ ±0
770 runs +1 768 ✔️ +1 2 💤 ±0 0 ❌ ±0

Results for commit 0ce4775. ± Comparison against base commit 0d0443d.

♻️ This comment has been updated with latest results.

… the type. Query system doesn't behave well with two types

burieberry

This is all good, but I think it would be fine to make an exception and not check isInsideAncestorChain when the type we're comparing to is the base carddef. isInsideAncestorChain will end up loading all the cards in the ancestor chain when we already know that all card subtypes descend from card def. In that case, you can just confirm via isCardDef for the subtype if we're not sure if the type is a card.

burieberry · 2025-02-07T16:24:47Z

packages/base/links-to-editor.gts

@@ -143,6 +147,7 @@ export class LinksToEditor extends GlimmerComponent<Signature> {

  private chooseCard = restartableTask(async () => {
    let type = identifyCard(this.args.field.card) ?? baseCardRef;
+    type = await getNarrowestType(this.args.subclassType, type, myLoader());


Alternatively, you can only call this function if a subclassType arg was provided

Actually do you even need to check if the type is the base CardDef? We know that all cards descend from it, so maybe in that case you can skip loading the whole chain and just use the provided subtype.

Actually do you even need to check if the type is the base CardDef? We know that all cards descend from it, so maybe in that case you can skip loading the whole chain and just use the provided subtype.

I think we do. We need to check if type is base CardDef as a terminating condition to "stop checking".

@burieberry maybe we can talk thru this after standup. Probably some small misunderstanding

@burieberry I have made the change that you have suggested

burieberry · 2025-02-13T15:14:31Z

packages/runtime-common/code-ref.ts

+async function isInsideAncestorChainOfCardDef(
+  codeRef: CodeRef,
+  loader: Loader,
+): Promise<boolean | undefined> {
+  let card = await loadCard(codeRef, { loader: loader });
+  if (isCardDef(card)) {
+    return true;
+  }
+  return false;
+}


I thought about this and I think it would be good to add to this some of your previous logic. This makes sense if the type we're comparing to is base CardDef, or if we only care that the subtype is a card def, regardless of what the type we're comparing to is. If type is not base card def, then we should consider if we want to confirm that subtype is descended from it. We currently only use this subtype argument for BoxelSpec, but making this change would allow us to use it in different cases where the type is not base card def. I'm open to hearing what other people in the team thinks about this.

For context, this is the diff change 02e8408. Previously, we use to have more generic handling of the subtype

habdelra · 2025-02-17T15:02:28Z

packages/runtime-common/code-ref.ts

@@ -227,3 +227,27 @@ export function humanReadable(ref: CodeRef): string {
 function assertNever(value: never) {
  return new Error(`should never happen ${value}`);
 }
+
+async function isInsideAncestorChainOfCardDef(


javascript has a similar kind of function called typeof. maybe that is a clearer name for this method: isTypeOf()

Actually, i take that back, this looks like a synonmym for isCardDef. let's just overload that specific method so it takes either a codeRef and loader and an already running card:

export function isCardDef(card: any): card is typeof CardDef; export function isCardDef(codeRef: CodeRef, loader: Loader): boolean export function isCardDef(cardOrCodeRef: any, loader?: Loader): boolean { ... }

(not too sure how all the type assertion works out since you can't assert a CodeRef param is a CarDef). if the overload doesn't work, at least try to name this is similar as you can to isCardDef, so that it is clear that we are using the same semantics.

Ahh ok I think this worked but loadCard is async so I used .then within the isCardDef implementation. I also added a note on using isCardDef in getNarrowestType

packages/host/tests/acceptance/code-submode/spec-test.gts

lukemelia · 2025-02-17T15:47:57Z

packages/base/card-api.gts

+        Named: {
+          format?: Format;
+          displayContainer?: boolean;
+          subclassType?: ResolvedCodeRef;


Perhaps typeConstraint or allowedClass would be a better name?

Co-authored-by: Luke Melia <luke@lukemelia.com>

burieberry

Looks much better

tintinthong marked this pull request as draft January 30, 2025 08:50

tintinthong changed the base branch from refactor-links-to-many-component to main February 3, 2025 02:37

tintinthong force-pushed the new-arg-to-filter-catalog-entry-type branch from ab3f885 to c8b1f13 Compare February 3, 2025 02:55

tintinthong added 3 commits February 5, 2025 12:30

add the addCardFilter arg

1f202c3

use subclassType rather than an additional filter and focus to narrow…

ce4ed57

… the type. Query system doesn't behave well with two types

fix ancestor logic

9f0fe1c

tintinthong force-pushed the new-arg-to-filter-catalog-entry-type branch from c8b1f13 to 9f0fe1c Compare February 5, 2025 04:30

tintinthong added 2 commits February 5, 2025 13:04

add test

b402924

fix lint

f69662b

tintinthong marked this pull request as ready for review February 5, 2025 08:37

revert

258dca8

tintinthong requested review from a team and removed request for a team February 5, 2025 08:47

tintinthong added 2 commits February 5, 2025 20:33

add proper stopping conditions

f0c74bb

improve stopping condition

9a66cd6

tintinthong changed the title ~~new arg to filter spec type~~ new arg to filter by subclass ref Feb 6, 2025

burieberry reviewed Feb 7, 2025

View reviewed changes

tintinthong added 2 commits February 11, 2025 16:16

getNarrowestType only if subclassType arg exists

bfb4749

Merge branch 'main' into new-arg-to-filter-catalog-entry-type

499f680

tintinthong requested review from burieberry and a team February 11, 2025 10:18

make isInsideAncestorChain less generic

02e8408

burieberry reviewed Feb 13, 2025

View reviewed changes

burieberry requested a review from a team February 13, 2025 15:37

habdelra reviewed Feb 17, 2025

View reviewed changes

lukemelia reviewed Feb 17, 2025

View reviewed changes

tintinthong and others added 3 commits February 18, 2025 11:55

Update test name to better english

9c8e743

Co-authored-by: Luke Melia <luke@lukemelia.com>

rename subclassType to typeConstraint

9b39afa

Use overload isCardDef

4e01ccc

tintinthong added 2 commits February 18, 2025 17:28

fix isCardDef with codeRef is async

00ecb24

Merge branch 'main' into new-arg-to-filter-catalog-entry-type

0ce4775

tintinthong requested review from lukemelia, burieberry, habdelra and a team February 18, 2025 11:55

lukemelia approved these changes Feb 18, 2025

View reviewed changes

burieberry approved these changes Feb 18, 2025

View reviewed changes

tintinthong merged commit ff33dc6 into main Feb 19, 2025
44 of 46 checks passed

tintinthong mentioned this pull request Feb 24, 2025

Allow field def containedExamples to render #2178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new arg to filter by subclass ref #2105

new arg to filter by subclass ref #2105

tintinthong commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading

burieberry left a comment •

edited

Loading

burieberry Feb 7, 2025

burieberry Feb 7, 2025

tintinthong Feb 11, 2025

tintinthong Feb 11, 2025

tintinthong Feb 13, 2025

burieberry Feb 13, 2025

tintinthong Feb 17, 2025

habdelra Feb 17, 2025

habdelra Feb 17, 2025 •

edited

Loading

tintinthong Feb 18, 2025 •

edited

Loading

lukemelia Feb 17, 2025

burieberry left a comment

new arg to filter by subclass ref #2105

new arg to filter by subclass ref #2105

Conversation

tintinthong commented Jan 30, 2025 • edited Loading

github-actions bot commented Jan 30, 2025 • edited Loading

Host Test Results

burieberry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habdelra Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

tintinthong Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burieberry left a comment

Choose a reason for hiding this comment

tintinthong commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading

burieberry left a comment •

edited

Loading

habdelra Feb 17, 2025 •

edited

Loading

tintinthong Feb 18, 2025 •

edited

Loading