Linting, Roll Migration

[JPMeehan]

• Attempted to resolve many linting issues
  • Did not touch some complicated files (common/abstract/data and common/abstract/document) nor files currently subject to PRs (prosemirror, shaders)
• Deleted the Roll-related files in client

Sidebar: Should we just... delete DataConfig and SourceConfig since those are from the v8/v9 configuration setup, pre-data models?

[LukeAbby]

In theory DataConfig and SourceConfig still serve a purpose for systems that have a template.json but go without a data model.

We could just roll it into DataModelConfig but DataConfig and SourceConfig are backwards compatible... but I'm not sure how valuable that is anymore.

[JPMeehan]

In theory DataConfig and SourceConfig still serve a purpose for systems that have a template.json but go without a data model.

We could just roll it into DataModelConfig but DataConfig and SourceConfig are backwards compatible... but I'm not sure how valuable that is anymore.

Currently, TypeDataField doesn't leverage those two setups. My inclination is that this project should focus on supporting Foundry's intended runtime validation, data models.

[LukeAbby]

As far as I understand it, TypeDataField will never leverage those two because TypeDataField is inherently for when a system actually sets up a data model. This is for the case where they have a template.json but not data models.

It honestly doesn't add much complexity to keep supporting. Some systems don't want to use data models, I say let them. If it were more complex I'd possibly be swayed by the argument that we're trying to maintain high quality and robustly typed code anyways but it's about as complex as adding something like DataConfig extends { readonly [_ in DocumentName]: { [_ in DocumentType]: infer DataModel } } ? DataModel : EmptyObject or something similar.

[JPMeehan]

TypeDataField is where the template.json gets used - see getInitialValue and _cleanType. On the server backend, where community packages can't register data models, they're functionally identical,

Anyways, if we want to keep supporting those I'm fine with that but we'll need to update the definition of the field to re-use DataConfig/SourceConfig. We may also want to double check how those are implemented because of the structural changes for documents.

[LukeAbby]

The old version of the signature actually served some utility. Specifically if you do something like:

class SomeApplication extends ApplicationV2<...> {
    override _renderHTML(...): Promise<HTMLElement>
}

const someApplication = new SomeApplication(...);

someApplication._replaceHTML(123, ...);

In the new signature this will simply compile fine because result: unknown. However with the older signature, result would be typed as HTMLElement.

[JPMeehan]

1. Old way has Private or protected member '_renderHTML' cannot be accessed on a type parameter.
2. I expect that all subclasses would more properly specify, e.g. HandlebarsApplicationMixin does this specification by fully overriding both of these functions.

[LukeAbby]

I know this wasn't you but I think the parameters starting with underscores, e.g. _context, _options, etc. should be set back to whatever it is in Foundry.

In this case I'm pretty sure it's just context and options. In my experience Foundry doesn't really add underscores to signify unused parameters.

[JPMeehan]

That's actually from Foundry this time!

[LukeAbby]

I've not dug into the code so this is a genuine question: makes you think _context can be literally anything? As in you could call with a function, or a symbol, or a class or so on.

[JPMeehan]

If you extend DialogV2 without replacing _prepareContext, the _context here will always be {}, which we've nominally covered with the generic at the top passing EmptyObject. Within the context of DialogV2 itself, the context is completely unused. It's a very unusual application class that does not have any additional rendering logic from handlebars, it's operating with pure strings and element creation.

The actual handling in Application##render is it calls const context = await this._prepareContext(options); and then passes that to a bunch of other functions like _preFirstRender, _renderHTML, etc; I'm sure that if you were to write a Svelte or React mixin you could absolutely have context be some other structure.

[LukeAbby]

Even if it's literally always empty I'd probably make a DialogV2.FirstRenderContext interface or something so that end users can extend it if need be. Same strategy needs to be applied as with ApplyOptions.

[JPMeehan]

This is all tracing from the first argument in the generics at the very top. Ultimately DialogV2 is not meant to be used with _prepareContext in part because there's no Handlebars support; everything dynamic here comes from this.options (the DialogV2.Configuration interface)

[LukeAbby]

I wanted to update ESLint so that no-empty-interface/no-empty-object-type wasn't going to cause problems.

I started the work in #2769
But that stalled once I realized that I'd have to migrate over to the new lints since the old ones seem to be completely unavailable?

[LukeAbby]

I've not been paying attention to dice stuff outside of the PRs I've seen for it. Remind me why it's okay to delete the dice files? Is it just because it's been completely removed in Foundry itself and superceded?

[JPMeehan]

#2660 added the files in their new location, client-esm/dice. Basically, foundry is in the process of moving everything to ESM; v13 is going to involve migrating almost all of the canvas and application classes from client to client-esm, although with applications they will probably leave any existing AppV1 subclasses within client and just have the replacements under client-esm.