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

introduce types.ts; improve types for internal functions with focus on hx-swap #2107

Closed
wants to merge 4 commits into from

Conversation

JLarky
Copy link
Contributor

@JLarky JLarky commented Dec 19, 2023

Description

previous PR

This PR will introduce a new file dist/types.ts into the mix, I'm not 100% sure if that's good or bad :)
So let's talk a bit about alternatives:

  • inline everything in .js file with jsdoc syntax --- that's what we were doing so far, not the best ergonomics
  • write types inside htmx.d.ts --- that works great for external types like HtmxConfig or html.addClass, but you don't want to put splitOnWhitespace there
  • write types inside types.d.ts --- this is what SvelteKit is doing and I first started with that approach when I started doing JsDoc projects, but I found it a bit clumsy, the biggest issue for me was how confusing it would work in case if you have both something.js and something.d.ts file
  • write types inside types.ts --- that's what someone recommended to me to try instead of doing .d.ts and I was happy ever since :D (but I'm not married to that)

So most of this PR is done, there are two outstanding things that I wanted to discuss before I turn this PR from Draft to Ready:

  • HtmxExtension.init --- this method takes HtmxInternalApi as first argument, so that means that HtmxInternalApi kind of have to be public API now, and if we are adding types of HtmxInternalApi to htmx.d.ts then you have to add all additional types like HtmxTriggerSpecification or TriggerHandler to it and so on.
    • so we could just use the type init(apiRef: import("./types").HtmxInternalApi): void; which works, but sends a signal that "there will be dragons"
    • I'm looking for feedback in terms of do we expect 3rd party extensions to consume those types (as in an npm module that will be written in TS that imports internal types to work) or even rewrite existing extensions to use those types, right now there's basically no type checkin in src/ext
  • how much of the stuff from src/types.ts should actually be in htmx.d.ts? In the original gist it feels like it was intended to be a part of public api https://gist.github.com/trvswgnr/7239fa78cd12cee986384623a29b9b89

Testing

 npm run test-types

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

| "afterend"
| "delete"
| "none";

Choose a reason for hiding this comment

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

we should probably add (string & NonNullable<unknown>) like this:

export type SwapStyle =
    | "innerHTML"
    | "outerHTML"
    | "afterbegin"
    | "beforebegin"
    | "afterend"
    | "beforeend"
    | "none"
    | "delete"
    | (string & NonNullable<unknown>);

because there are other swap styles (like morph:innerHTML when using idiomorph)

this way, editor autocomplete will still be there for known values, but any string can be passed.

@trvswgnr
Copy link

you can add me as co-author on this 😘



Co-authored-by: trav <[email protected]>

@JLarky
Copy link
Contributor Author

JLarky commented Dec 19, 2023

you can add me as co-author on this 😘

will do :) do you have any thoughts on types.ts alternatives? :)

@trvswgnr
Copy link

trvswgnr commented Dec 19, 2023

you can add me as co-author on this 😘

will do :) do you have any thoughts on types.ts alternatives? :)

i think that devs authoring their own extensions will usually do so in their own environment and not as part of the htmx project (at least initially), so it would make sense that those types are exposed somehow. "internal api" is a little misleading, as it gets used throughout extensions, making it more of a "developer api".

the only reason i would be for .d.ts instead of .ts is that the latter might lead people to believe that there is logic in there.

threshold?: string;
delay?: number;
pollInterval?: number;
}

Choose a reason for hiding this comment

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

looked through and updated this with keys found in source:

export interface HtmxTriggerSpecification {
    trigger: string;
    sseEvent?: string;
    eventFilter?: string;
    changed?: boolean;
    once?: boolean;
    consume?: boolean;
    from?: string;
    target?: string;
    throttle?: number;
    queue?: string;
    root?: string;
    threshold?: string;
    delay?: number;
    pollInterval?: number;
};

@alexpetros
Copy link
Collaborator

Hey all, we're about to do a pretty significant refactor and unfortunately now isn't the right time to introduce this. The htmx typing story is at the moment incomplete—if you want to update the API types you're more than welcome to.

@alexpetros alexpetros closed this Dec 20, 2023
@JLarky
Copy link
Contributor Author

JLarky commented Dec 21, 2023

@alexpetros I need a bit more guidance here :)

@alexpetros
Copy link
Collaborator

alexpetros commented Dec 21, 2023

Haha the guidance is, for the moment, please hold off on TS changes.

EDIT: unless they're to the type declarations in the API interface.

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.

3 participants