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

Modernize components and prepare for v2 #61

Merged
merged 13 commits into from
Mar 20, 2024
Merged

Modernize components and prepare for v2 #61

merged 13 commits into from
Mar 20, 2024

Conversation

dfreeman
Copy link
Member

Background

#58, #59 and #60 together undertook a fairly major overhaul of Exclaim's internals while intentionally not touching the parts of this repo that were not directly affected by those changes. This left a number of 'obvious next steps' and opportunities for cleanup undone.

This Change

This PR takes care of the items called out under "Not This Change" over the course of the previous updates:

  • Adding canonical .d.ts files to cover Exclaim's public APIs
  • Cleaning up some inconsistencies and weirdness that had built up in the shape of the @implementationMap arg over the course of v1
  • Making sure we're using modern patterns and base classes for components and reactivity in Exclaim itself, as well as the playground app we use to demo it
  • Updating the README and GLOSSARY documents to clear up any outdated information, as well as adding notes about breaking changes from v1 to v2 at the foot of the README.

@jamescdavis
Copy link
Contributor

Don't rely on @cached

lol, was about to request changes for this

/**
* A spec for an Exclaim UI.
*/
ui: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to try to type this at all? At the very least, it would be Record<string, unknown>, right? I've attempted some more advanced recursive types for this in the past, but at this point not sure if something like that makes any sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hot take: imho Record<string, unknown> is worse than unknown for cases like this. It does rule out "fundamental misunderstanding" bugs like @ui={{123}}, but realistically those aren't the kinds of errors that are likely to be present in a UI config object. Record<string, unknown> suggests some degree of type safety at least at the top level where really none exists.

You can go super deep on deriving approximate valid types for a UI config based on an implementation map (particularly if you assume some domain-specific understanding of the contents of meta), but ultimately the reality is that if you actually statically know the type of your UI config, you don't have any reason to be using Exclaim and should probably invoke your components directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fair and good points. Ultimately, UI config is usually sourced from something dynamic and untyped, like JSON (otherwise, like you say, what's the point of using exclaim) so it makes sense that we should add no artificial type restriction here.

Copy link
Contributor

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

:shipit:

@dfreeman dfreeman merged commit f1b503b into main Mar 20, 2024
9 checks passed
@dfreeman dfreeman deleted the modernize branch March 20, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants