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

fix: type definitions for HtmxExtension #2721

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

cngJo
Copy link
Contributor

@cngJo cngJo commented Jul 13, 2024

The @see tag prevented tsc to properly generate types for HtmxExtension. Moving the @see above @typedef fixes this and adds types for HtmxExtension back to the package ;-)

Description

#2336 updated type generation but as a side effect the type definition of HtmxExtension got dropped / changed to any.

This is fixed now ;)

Note: I did not create an issue to prevent spam, if it's important, I'm more than happy to create one after the fact 😉

Testing

Please explain how you tested this change manually, and, if applicable, what new tests you added. If
you're making a change to just the website, you can omit this section.

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

@cngJo cngJo force-pushed the fix/htmxextension-types branch 2 times, most recently from 36be96a to 1132079 Compare July 13, 2024 11:28
@MichaelWest22
Copy link
Contributor

I found a missing new extension field

when i was working on #2711 so this may need to be added in. This was added a few months ago for the SSE extension but it may not have been updated fully in the extension repo as well i'm thinking

@cngJo
Copy link
Contributor Author

cngJo commented Jul 13, 2024

The two errors in the test are related to handleSwap on HtmxExtension being expected to return an array of elements instead of boolean what the type suggests.

I took a look at the implementations at htmx-extensions for handleSwap and saw both usages:

What would the expected behavior be? I'd offer to update htmx-extensions with that info as well.

@cngJo
Copy link
Contributor Author

cngJo commented Jul 13, 2024

@MichaelWest22 thanks for noticing! I found it when checking the failing tests and just force-pushed a fix, adding it to the HtmxExtension type.

Edit: forgot to update src/htmx.d.ts from dist/htmx.d.ts that's fixed now as well.

@cngJo cngJo force-pushed the fix/htmxextension-types branch from 1132079 to e765568 Compare July 13, 2024 11:36
@MichaelWest22
Copy link
Contributor

Also the see web link is now linking to 404 after htmx 2 upgrade and the split from to a new extension. maybe this see link needs to link to https://github.com/bigskysoftware/htmx-extensions/blob/main/README.md instead maybe since I can't see any other place this info is now. the info here also probably needs updating...

@cngJo cngJo force-pushed the fix/htmxextension-types branch from e765568 to 1c947d9 Compare July 13, 2024 15:30
@cngJo
Copy link
Contributor Author

cngJo commented Jul 13, 2024

There is https://htmx.org/docs/#extensions, but itself just points to the README you linked. I've updated the link accordingly and going to file a PR against https://github.com/bigskysoftware/htmx-extensions to update the type information in the README as well.

@Telroshan Telroshan added bug Something isn't working 2.0 documentation Improvements or additions to documentation and removed bug Something isn't working labels Jul 13, 2024
src/htmx.js Outdated Show resolved Hide resolved
@cngJo cngJo force-pushed the fix/htmxextension-types branch 2 times, most recently from 62e0162 to b33acce Compare July 13, 2024 16:41
@cngJo
Copy link
Contributor Author

cngJo commented Jul 13, 2024

@Telroshan I updated the return type of getSelectors to also allow null, as per bigskysoftware/htmx-extensions#49 (comment)

@Telroshan
Copy link
Collaborator

Perfect thanks! Last request; could you remove the changes to dist as per the contributing guidelines ? We generate those files when cutting a new release

@cngJo cngJo force-pushed the fix/htmxextension-types branch from b33acce to cfcdeac Compare July 13, 2024 17:19
@cngJo cngJo force-pushed the fix/htmxextension-types branch from cfcdeac to 815a466 Compare July 13, 2024 17:22
@cngJo
Copy link
Contributor Author

cngJo commented Jul 13, 2024

Oh, sorry ... but sure - I removed the changes from the dist/ directory :-)

@Telroshan Telroshan merged commit cbb2b46 into bigskysoftware:dev Jul 13, 2024
1 check passed
@Telroshan
Copy link
Collaborator

Thanks!

@maddalax
Copy link
Contributor

@cngJo Hi, just curious, was it intentional that each field on HtmxExtension is required with the new type? I believe on HTMX v1, just the onEvent was sufficient for creating a new extension.

With this change, TS forces you to add each field to the extension obj, even if you aren't needing to override it

@Telroshan
Copy link
Collaborator

Hey, nope that's an oversight @maddalax indeed

Though the issue isn't that PR in itself (that introduced back the types for HtmxExtension that was a any before it).
The issue is in the JSDoc of HtmxExtension that declares the properties.
The thing is, when using an extension, all those properties will indeed be set and called by htmx, thus are mandatory, but when defining an extension, htmx merges your passed object with the extension base object.

Maybe we should declare a separate JSDoc for the defineExtension where the properties are optional, but just for this function, as for any other usage of extensions, we expect those properties to be well defined.

If you'd like to investigate into it and submit a fix PR @maddalax, please do! PR welcome

See

htmx/src/htmx.js

Lines 4805 to 4834 in e6a2ea1

/**
* extensionBase defines the default functions for all extensions.
* @returns {HtmxExtension}
*/
function extensionBase() {
return {
init: function(api) { return null },
getSelectors: function() { return null },
onEvent: function(name, evt) { return true },
transformResponse: function(text, xhr, elt) { return text },
isInlineSwap: function(swapStyle) { return false },
handleSwap: function(swapStyle, target, fragment, settleInfo) { return false },
encodeParameters: function(xhr, parameters, elt) { return null }
}
}
/**
* defineExtension initializes the extension and adds it to the htmx registry
*
* @see https://htmx.org/api/#defineExtension
*
* @param {string} name the extension name
* @param {HtmxExtension} extension the extension definition
*/
function defineExtension(name, extension) {
if (extension.init) {
extension.init(internalAPI)
}
extensions[name] = mergeObjects(extensionBase(), extension)
}

@maddalax
Copy link
Contributor

Hey, nope that's an oversight @maddalax indeed

Though the issue isn't that PR in itself (that introduced back the types for HtmxExtension that was a any before it). The issue is in the JSDoc of HtmxExtension that declares the properties. The thing is, when using an extension, all those properties will indeed be set and called by htmx, thus are mandatory, but when defining an extension, htmx merges your passed object with the extension base object.

Maybe we should declare a separate JSDoc for the defineExtension where the properties are optional, but just for this function, as for any other usage of extensions, we expect those properties to be well defined.

If you'd like to investigate into it and submit a fix PR @maddalax, please do! PR welcome

See

htmx/src/htmx.js

Lines 4805 to 4834 in e6a2ea1

/**
* extensionBase defines the default functions for all extensions.
* @returns {HtmxExtension}
*/
function extensionBase() {
return {
init: function(api) { return null },
getSelectors: function() { return null },
onEvent: function(name, evt) { return true },
transformResponse: function(text, xhr, elt) { return text },
isInlineSwap: function(swapStyle) { return false },
handleSwap: function(swapStyle, target, fragment, settleInfo) { return false },
encodeParameters: function(xhr, parameters, elt) { return null }
}
}
/**
* defineExtension initializes the extension and adds it to the htmx registry
*
* @see https://htmx.org/api/#defineExtension
*
* @param {string} name the extension name
* @param {HtmxExtension} extension the extension definition
*/
function defineExtension(name, extension) {
if (extension.init) {
extension.init(internalAPI)
}
extensions[name] = mergeObjects(extensionBase(), extension)
}

Ah gotcha sorry I skipped over the part that it was auto generated. I may have some time to investigate this today, will see if I can come up with anything.

Thanks for letting me know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants