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

Experimental "propertyAutocomplete" for sync tables #2463

Merged
merged 21 commits into from
Mar 14, 2023

Conversation

dweitzman-codaio
Copy link
Contributor

@dweitzman-codaio dweitzman-codaio commented Feb 17, 2023

I'm trying to keep this minimal to get started. Some notes:

You would have to set autocomplete: true on a property to opt-in to calling the propertyAutocomplete function on the sync table.

There's a snag with this: Email properties already have an autocomplete option that I think implies completing emails known to Coda. The current approach of this PR is to reuse the same "autocomplete: boolean" setting and declare that Email properties don't support 2-way autocomplete (but switching them to a non-Email string property would work fine for pack property autocomplete).

I tried to add some validation that you can't set "autocomplete: true" and "mutable: false", but it's imperfect and doesn't work in all scenarios. An alternative would be to say the "autocomplete: true" implies "mutable: true" rather than making you explicitly set both (except for "Email" properties?...).

The user-written propertyAutocomplete method current returns type Promise<any[]> | any[], which raises two issues:

  • There's nothing to gently discourage pack authors from returning results of the wrong type for the column. With dynamic schemas that seems unavoidable
  • This PR doesn't offer any solution yet to how do things like return non-object results that have both a display name and value like parameters do. For example, maybe you want to search over project names but fill in a project ID that isn't human readable. The lack of return types makes this muddier. Some kind of coda.makeAutocompleteResult() function could return objects that are easy to identify at runtime, like [{'__codaType': 'displayName', 'displayName': "Bob", 'userId': 1234}]
  • This PR doesn't offer any solution to how a pack author would communicate if results are safe to cache for some length of time, and the lack of typing makes that muddier. Some kind of dynamic typing could potentially address this, or we might decide later to wrap the results in an object like {results: [...], cacheTtlSecs: 60} instead of returning raw JS arrays

Select lists with options hard-coded into the schema are not implemented in this PR. Seems simple enough to add that as a separate PR.

- Make CellAutocompleteExecutionContext as a sub-interface of ExecutionContext, eliminating the AdditionalMetadataContext
- Start adding support for cache key tracking by using Object.defineProperty() to track which sync table properties were accessed
@@ -6945,6 +6945,31 @@ module.exports = (() => {
return parentFormula.execute(params, executionContext);
}
break;
case "CellAutocomplete" /* CellAutocomplete */:
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 unusual. There's hackery here both to set up the execution context with additional properties and to observe which properties are read and stuff them secretly into the formula result before returning

api.ts Outdated
@@ -1656,6 +1670,8 @@ export function makeSyncTable<
...definition
} = maybeRewriteConnectionForFormula(formula, connectionRequirement);

const wrappedAutocomplete = autocomplete ? makeMetadataFormula(autocomplete as MetadataFunction) : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure makeMetadataFormula will return the right thing as-is. I think it'll return a formula with different parameters than your new interface. So probably need a new wrapper analogous to makeMetadataFormula but using correct params. Probably implies it would be best to update the typing for autocompleteCell to a new variant of MetadataFormula as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return {
result,
// TODO(dweitzman): Keys used should be an object or array, not a string
cacheKey: `keys used: ${cacheKeysUsed.join(',')}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why we need to return cache keys here. Isn't it up to the client to keep track of what permutations of request params have already been used and cache them locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client passes all parameters to the pack, with no way of knowing which parameters were actually used unless the pack returns that information back

An alternative approach would be to have the pack formula pre-declare somehow in the schema which properties might influence the autocomplete of each other property. With that approach people would have to manually update their dependencies, and mistakes would lead to bugs.

api.ts Outdated Show resolved Hide resolved
api.ts Outdated
Comment on lines 1228 to 1231
export type CellAutocompleteMetadataFunction = (
context: CellAutocompleteExecutionContext,
search: string,
formulaContext?: MetadataContext,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with Eric that we probably need the sync table params somewhere in here, for the same reasons that @chris-codaio decided we need them in the update() function. Not sure the clearest interface for that, but should be able to use the type used in the update() function, and either make it a new param here or dangle it off one of the contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the bigger issue is that the sync table params can vary depending on which sync step caused this row to appear, and I don't think we currently have a way to figure that out.

Hypothetically maybe we could add a new column to the source table to track which sync step is responsible for its appearance?...

cc @huayang-codaio

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think you need that for 2-way.

export interface CellAutocompleteExecutionContext extends ExecutionContext {
readonly propertyName: string;
readonly propertyValues: {[propertyValues: string]: any};
readonly search: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

For parameter auto-complete the search value is passed as a parameter to the autocomplete function:

autocomplete: async function (context, search, parameters) {
  // ...
},

For consistency I'd recommend doing the same for cell auto-complete. I'm not opposed to also including it in the context, if that is what you are doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for including it in the context here is to do the implicit caching trickery where we can see if the search string was used or not and factor that into the cache key.

With a parameter I'm not sure if that'd be possible, but with a property we can use Object.defineProperty() to spy on reads

api.ts Outdated
@@ -1222,6 +1225,10 @@ export type MetadataFunction = (
formulaContext?: MetadataContext,
) => Promise<MetadataFormulaResultType | MetadataFormulaResultType[] | ArraySchema | ObjectSchema<any, any>>;

export type CellAutocompleteMetadataFunction = (
context: CellAutocompleteExecutionContext,
) => Promise<MetadataFormulaResultType | MetadataFormulaResultType[] | ArraySchema | ObjectSchema<any, any>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably tighten this up--these functions don't need to return anything schema-related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -761,6 +761,12 @@ export interface SyncExecutionContext extends ExecutionContext {
readonly sync: Sync;
}

export interface CellAutocompleteExecutionContext extends ExecutionContext {
readonly propertyName: 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 is the name of the field that corresponds to the cell being edited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this indicates the column being edited.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will need to run through untransformKeys or some equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -761,6 +761,12 @@ export interface SyncExecutionContext extends ExecutionContext {
readonly sync: Sync;
}

export interface CellAutocompleteExecutionContext extends ExecutionContext {
readonly propertyName: string;
readonly propertyValues: {[propertyValues: string]: any};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, for the unused key alias, otherPropertyName or something (the dict keys are names and not values).

api.ts Outdated
@@ -1619,6 +1628,8 @@ export interface DynamicSyncTableOptions<
* in placeholderSchema will be rendered by default after the sync.
*/
placeholderSchema?: SchemaT;

autocomplete?: CellAutocompleteMetadataFunction;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason the compiled property is called cellAutocomplete but this isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this to propertyAutocomplete

api.ts Outdated
@@ -283,6 +285,7 @@ export interface DynamicSyncTableDef<
getDisplayUrl: MetadataFormula;
/** See {@link DynamicSyncTableOptions.listDynamicUrls} */
listDynamicUrls?: MetadataFormula;
autocompleteCell?: MetadataFormula;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this type is accurate if it accepts your new context type and not the usual parameters of an autocomplete formula.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I still need to fix this. I'll make a MetadataFormulaWithoutParams in case some other formula wants to use ExecutionContext instead of parameters similarly in the future

return {
result,
propertiesUsed: cacheKeysUsed,
} as any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll want to type this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still would like to at least declare a type for what the thunk is supposed to return for PropertyAutocomplete. And I suspect we would use or reference that type on the coda side when handling the results of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some types for the return value. We may need to iterate on it in the future, but as a first crack it looks like this:

/**
 * These will be created with a helper function.
 */
interface PropertyAutocompleteFormattedResult {
  __brand: 'PropertyAutocompleteSimpleResult';
  /** Text that will be displayed to the user in UI for this option. */
  display: string;
  /** The actual value for this option */
  value: any;
}

type PropertyAutocompleteResults =
  | any[]
  | {
      cacheTtlSecs?: number;
      results: Array<any | PropertyAutocompleteFormattedResult>;
    };

/**
 * @hidden
 */
export interface PropertyAutocompleteAnnotatedResult {
  packResult: PropertyAutocompleteResults;
  propertiesUsed: string[];
}

This doesn't address the question of how to distinguish PropertyAutocompleteFormattedResult from a pack object that also has fields named "display" and "value". Seems like distinguish those cases we'd need a helper function either to wrap individual results (and fill in the __brand value, maybe?) or to wrap the entire result list.

api.ts Outdated
@@ -283,6 +285,7 @@ export interface DynamicSyncTableDef<
getDisplayUrl: MetadataFormula;
/** See {@link DynamicSyncTableOptions.listDynamicUrls} */
listDynamicUrls?: MetadataFormula;
autocompleteCell?: MetadataFormula;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary since the base type already has this?

@@ -761,6 +761,12 @@ export interface SyncExecutionContext extends ExecutionContext {
readonly sync: Sync;
}

export interface CellAutocompleteExecutionContext extends ExecutionContext {
readonly propertyName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will need to run through untransformKeys or some equivalent.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

This pull request has been inactive for 7 days: labeled as stale. To avoid auto-closure in 7 days, please do one of the following: Add keep-active label, comment on PR, push new commit on PR.

@dweitzman-codaio dweitzman-codaio changed the title Prototype of SDK change for cell autocomplete Experimental "propertyAutocomplete" for sync tables Mar 10, 2023
@dweitzman-codaio
Copy link
Contributor Author

I removed select lists from this PR, relaxed some of the typing, and tried to clean things up a bit and add a test or two

The description is updated to note some things that are awkward or not addressed by the PR.

@dweitzman-codaio dweitzman-codaio marked this pull request as ready for review March 10, 2023 01:30
@dweitzman-codaio dweitzman-codaio requested review from a team as code owners March 10, 2023 01:30
api.ts Outdated
* See {@link SyncTableOptions.propertyAutocomplete}
* @hidden
*/
propertyAutocomplete?: MetadataFormula;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The naming proposed here is "propertyAutocomplete" for this type of autocomplete, vs parameter autocomplete for the kind we already have

If you imagine a future where we let people "autocomplete" pack objects directly into a canvas then maybe "schemaAutocomplete" would be a more accurate/flexible name, but that would feel kind of abstract

L extends string,
SchemaT extends ObjectSchemaDefinition<K, L>,
> = ObjectSchemaDefinitionType<K, L, SchemaT> | Error;
export type SyncUpdateSingleResult<K extends string, L extends string, SchemaT extends ObjectSchemaDefinition<K, L>> =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My vscode wanted to change some formatting


/**
* Includes the unreleased propertyAutocomplete parameter.
* @hidden
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used function overloading + @hidden here to avoid propertyAutocomplete appearing in documentation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah hmm, this is fine but I thought you could just put @hidden inline on a property e.g. like above line 1921 and it would suppress that property from documentation.

Copy link
Contributor Author

@dweitzman-codaio dweitzman-codaio Mar 10, 2023

Choose a reason for hiding this comment

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

Seems like it's possible to extract an exported MakeDynamicSyncTableParams type where one of its fields is @hidden, but it has consequences for the documentation. I poked around for a few minutes, but I think this temporary code duplication is probably the simplest and least disruptive option I tried

Copy link
Collaborator

@jonathan-codaio jonathan-codaio left a comment

Choose a reason for hiding this comment

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

Cool, looking great overall.

@@ -1271,6 +1282,10 @@ export type MetadataFormula = BaseFormula<[ParamDef<Type.string>, ParamDef<Type.
schema?: any;
};

type PropertyAutocompleteMetadataFormula = BaseFormula<[], any> & {
schema?: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there not a generic schema type we can use here?

Oh, is this just optionalizing schema?

Copy link
Contributor Author

@dweitzman-codaio dweitzman-codaio Mar 13, 2023

Choose a reason for hiding this comment

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

This was copy-pasted from the definition of export type MetadataFormula above. On closer inspection, seems like the typing in the coda repo is only working here by chance: it's using MetadataFormulaMetadata below for metadata formulas even though PropertyAutocompleteMetadataFormula isn't quite the same as MetadataFormula.

I need to look into the typing here a bit more. The schema here isn't needed by anything in this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've introduced two new types, GenericMetadataFormula and GenericMetadataFormulaMetadata, to be used from the coda repo in place of MetadataFormula and MetadataFormulaMetadata.

The impact is basically that the types no longer imply any knowledge about what parameters specific a given metadata formula takes as arguments, and the coda repo is just essentially trusted to do the right thing. There wasn't any real type safety or checking around that before anyway in _maybeExecutePackFormula

options?: {connectionRequirement?: ConnectionRequirement},
): PropertyAutocompleteMetadataFormula {
if (!(execute instanceof Function)) {
throw new Error(`Value for propertyAutocomplete must be a function`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm mostly fine with this, just noting it's inconsistent with wrapMetadataFunction(), which allows you to pass a pre-wrapped object formula def too. But I guess that's more for legacy reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it wasn't clear to me that there was any benefit to letting people define their own formula explicitly so I figured it's simpler not to support that. We can always allow people to manually define their own PropertyAutocompleteMetadataFormula in the future if we find a use case


/**
* Includes the unreleased propertyAutocomplete parameter.
* @hidden
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah hmm, this is fine but I thought you could just put @hidden inline on a property e.g. like above line 1921 and it would suppress that property from documentation.

return {
result,
propertiesUsed: cacheKeysUsed,
} as any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still would like to at least declare a type for what the thunk is supposed to return for PropertyAutocomplete. And I suspect we would use or reference that type on the coda side when handling the results of these.

*
* @hidden
*/
autocomplete?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the comment should probably indicate that this property only makes sense when mutable is true, and we should enforce accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1178,6 +1180,7 @@ const baseSyncTableSchema = {
getter: syncFormulaSchema,
entityName: z.string().optional(),
defaultAddDynamicColumns: z.boolean().optional(),
propertyAutocomplete: objectPackFormulaSchema.optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the existing validator objectPackFormulaSchema going to fail on this since it doesn't have a schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works, and it's based on what's done for the getSchema metadata formula (schema is optional for objectPackFormulaSchema). Some relevant bits of code from the getSchema case:

  getSchema: formulaMetadataSchema.optional(),


const formulaMetadataSchema = z
  .union([numericPackFormulaSchema, stringPackFormulaSchema, booleanPackFormulaSchema, objectPackFormulaSchema])

const objectPackFormulaSchema = zodCompleteObject<Omit<ObjectPackFormula<any, any>, 'execute'>>({
  ...commonPackFormulaSchema,
  resultType: zodDiscriminant(Type.object),
  // TODO(jonathan): See if we should really allow this. The SDK right now explicitly tolerates an undefined
  // schema for objects, but that doesn't seem like a use case we actually want to support.
  schema: z.union([genericObjectSchema, arrayPropertySchema]).optional(),
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah interesting, my fault then for leaving this as optional this entire time. Well, cool then, just make sure your case is covered by a unittest so that if this TODO is ever resolved and the schema becomes required for top-level formulas, that it doesn't break autocomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified that tests in upload_test.ts will fail if the schema becomes non-optional in the future.

  2) Pack metadata Validation
       Formulas
         sync tables
           valid sync table:
     AssertionError: Expected validation to succeed but failed with error [{"path":"syncTables[0].propertyAutocomplete.schema","message":"Required"}]
      at validateJson (/Users/DavidWeitzman/src/packs-sdk/test/upload_validation_test.ts:56:14)
      at async Context.<anonymous> (/Users/DavidWeitzman/src/packs-sdk/test/upload_validation_test.ts:798:9)

| any[]
| {
cacheTtlSecs?: number;
results: Array<any | PropertyAutocompleteFormattedResult>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the any | here? If you're going to return a formatted result object, might as well require that the individual results are also all formatted too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And similar here, if the pack returns scalars the SDK should convert them to PropertyAutocompleteFormattedResult and then we should get a strict type here.


Object.defineProperty(cellAutocompleteExecutionContext, 'search', {
get() {
recordPropertyAccess('__search');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder if __search should be a named constant, since presumably you're going to reference it on the coda side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forgot I'd done that. The one other place this constant appears in the prototype is strictly for implementing the caching.

Another option here could be to record the property access in a totally different way so the output looks more like {results: [...], propertiesUsed: ['foo'], searchUsed: true}

The cache implementation might still want to try to use strings as cache keys, but then the problem of converting this to and from a string would be localized there.

There are other things that could impact caching that aren't implemented here (like a change to dynamic schema) but I suspect the easiest way to deal with those is to just clear the cache for any sync table if the connection or schema changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code to use a dedicated property to indicate if the search string was read instead of a magic constant within propertiesUsed

Comment on lines +1290 to +1292
* These will be created with a helper function.
*/
interface PropertyAutocompleteFormattedResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this helper not implemented yet? I'm missing where the object of this shape is constructed from the results that the pack maker returns. I guess I would have expected it to happen in the thunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I haven't implement a helper yet, just improvising this type in the PR. Not sure exactly how things should look on the pack side, from an api-perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the pack should be able to return any. Seems like it should follow the example of regular autocomplete, where you can either return a scalar, or you can return a {display, value} object if you want to do something fancy. Wouldn't we want them to explicit return a display string when returning an object, too?

If so, I'd suggest that PropertyAutocompleteMetadataFunction returns string | number | boolean | PropertyAutocompleteFormattedResult.

Copy link
Contributor Author

@dweitzman-codaio dweitzman-codaio Mar 13, 2023

Choose a reason for hiding this comment

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

Parameter autocomplete only supports string | number or one annotated with a display name as the values, while property autocomplete here is aspiring to support at least three scenarios:

  1. Scalars like string, number and potentially Dates or booleans (like parameter autocomplete)
  2. Scalars with user-friendly names like {display: 'John', value: 'johnsUserId1234'} (like parameter autocomplete)
  3. Objects like {someProperty: 'someValue', 'anotherProperty': 123} (unlike parameter autocomplete). Potentially even using string display names?...

The challenge is distinguishing between (2) and (3). One way to handle that could be like this:

interface SimplePropertyAutocompleteResults {
  type: 'Simple',
  cacheTtlSecs?: number;
  results: Array<any>;
}
interface AnnotatedPropertyAutocompleteResults {
  type: 'Annotated',
  cacheTtlSecs?: number;
  results: Array<PropertyAutocompleteFormattedResult>;
}
type PropertyAutocompleteResults = SimplePropertyAutocompleteResults | AnnotatedPropertyAutocompleteResults;

A tradeoff there would be that with this approach we'd probably want people to use a helper function most of the time in the pack, something like this:

  return coda.makeSimplePropertyAutocompleteResults({cacheTtlSecs: 3}, ['foo', 'bar']);

  // With display values
  return coda.makeAnnotatedPropertyAutocompleteResults([
    {display: 'foo', value: 'fooId123'},
    {display: 'bar', value: 'barId456'},
  ]);

That's probably fine?... cc @ekoleda-codaio for your thoughts

I was originally trying to make it so that in the simplest case you could just return ['foo', 'bar'] without calling a helper function, which would mean more runtime guessing about whether something is or isn't a PropertyAutocompleteFormattedResult

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just say that for (3) you have to return {display: 'someDisplay', value: {someProperty: 'someValue', 'anotherProperty': 123}} and take the guesswork out of it?

It seems like we probably need an explicit display value when the pack is returning an object value result anyway, for various render scenarios, in which case we have to use this result format anyway?

Copy link
Contributor Author

@dweitzman-codaio dweitzman-codaio Mar 14, 2023

Choose a reason for hiding this comment

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

You don't necessarily need an explicit separate display value for returning an object. For a concrete example, here's pack prototype code for autocompleting Jira Users (and Statuses) in formulas.ts: https://github.com/coda/packs/compare/dweitzman-jira-autocomplete

The returned autocomplete result for assignee (a User property) has type SchemaType<typeof User>[], where User is the makeObjectSchema for a user, so it's returning a result like [{displayName: 'John', accountId: 'abc123'}]

The prototype autocomplete UI doesn't support chip rendering in the dropdown yet but knows what text to render (John) from the column schema since User has displayProperty: 'displayName'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok. So it sounds like the legal returns values are scalar OR {display, value} OR object that matches the object schema of this property in the row schema?

What are the consequences if we see {display, value} and interpret it as an annotated display value rather than an object? If it's not consequential then maybe it doesn't matter if we just guess. If we know the schema we're looking for, maybe it's even easier to guess well.

Otherwise I don't think it's unreasonable to have to wrap one of the object types to disambiguate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably do a pretty good job of guessing

If an object has only display and value as properties, we could look at the column's expects schema and see if display and value are both valid properties on the schema where one is displayProperty and the other is idProperty. If not, we could safely assume we're looking at an object that's annotated for display in autocomplete

Copy link
Collaborator

@jonathan-codaio jonathan-codaio left a comment

Choose a reason for hiding this comment

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

Gonna approve to unblock, I think we can iterate on the result value shape separately.

Comment on lines +1290 to +1292
* These will be created with a helper function.
*/
interface PropertyAutocompleteFormattedResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok. So it sounds like the legal returns values are scalar OR {display, value} OR object that matches the object schema of this property in the row schema?

What are the consequences if we see {display, value} and interpret it as an annotated display value rather than an object? If it's not consequential then maybe it doesn't matter if we just guess. If we know the schema we're looking for, maybe it's even easier to guess well.

Otherwise I don't think it's unreasonable to have to wrap one of the object types to disambiguate.

@dweitzman-codaio
Copy link
Contributor Author

Gonna approve to unblock, I think we can iterate on the result value shape separately.

Sounds good. Seems likely we'll want to make changes after trying it out a few different packs.

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

5 participants