Simplify resolution and support plain function helpers in GlimmerX #54
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This temporarily breaks `on` and `fn` as they can no longer cleanly merge with their upstream declarations, which will force the issue we've been putting off in #25c
chriskrycho
reviewed
Mar 7, 2021
| expectTypeOf(resolveOrReturn({} as never)).toEqualTypeOf<any>(); | ||
| expectTypeOf(resolveOrReturn({} as never)).toEqualTypeOf<never>(); |
chriskrycho
reviewed
Mar 7, 2021
Comment on lines
+20
to
+26
| on: Invokable< | ||
| <T extends keyof HTMLElementEventMap>( | ||
| args: NoNamedArgs, | ||
| event: T, | ||
| callback: (event: HTMLElementEventMap[T]) => void | ||
| ) => CreatesModifier | ||
| >; |
There was a problem hiding this comment.
Question for my illumination: how would we wire this up to a set of allowed events for a given DOM element (later, obviously; was just thinking about it when reading the change here)?
There was a problem hiding this comment.
As far as I know TS's own DOM lib doesn't track that information, and with modifiers as generic as they are, I'm not sure if we could express that constraint in a way that would produce useful autosuggest for users even if the types were available.
For the slightly more general question of enforcing that modifiers are only applied to elements they'll work with, the way things are currently set up I think we'd wind up with something like:
function modifier<El extends Element, Positional extends unknown[], Named>(
f: (element: El, positional: Positional, named: Named): void | () => void
): new () => Invokable<(args: Named, ...positional: Positional) => CreatesModifier<Element>>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This change includes the necessary updates to our invokable resolution process to support treating plain functions as helpers in GlimmerX, ultimately simplifying baseline resolution down to two cases in the process.
Details
The key change is that we now never have free-floating function types being passed around in their Glint-internal format. Instead, invokables are always wrapped as wither
Invokable<T>ornew (...) => Invokable<T>, depending on the use case.The comment laid out in
packages/template/-private/resolution.d.tsgoes into further detail, but standaloneInvokable<T>format allows us to resolve complex hand-declared items likefnwithout losing type fidelity, whilenew () => Invokable<T>is a natural fit for things like component classes that we already traffic heavily in.The
newversion also allows for the preservation of type parameters on the return side of functions that produce invokables, likehelper()and{{component}}. Previously we were using regular functions to support this case, which was what led to the difficulty of supporting plain functions.Notes
This notably breaks
fnandonfor GlimmerX, which are now temporarily declared asfn2andon2to avoid clashing with the built-in definitions offnandonfrom GlimmerX itself. Previously we were able to get by with declaration merging, but the way we treat these values is now sufficiently diverged from the baseline definitions that that's no longer possible.This is already an issue we've been skirting as outlined in #25, and at this point I think the simplest path forward for the time being is to expose modules from the environment packages that directly reexport the same runtime values but with glint-compatible type declarations.
This will also give us the freedom to experiment with alternative form factors for e.g. component type parameters in a way that consumers can try out today rather than having to roll out breaking changes upstream first.