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

Add JSDoc types and definitions #25

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mkarajohn
Copy link

@mkarajohn mkarajohn commented Dec 21, 2023

Added types to every function and config object, to the best of my ability.

One thing that surfaced from this process is that in some places functions expect arguments as Element, but the functions themselves may have been called in places where the arguments that are passed are of the broader Node type, not Element.

This could lead to bugs theoretically (big if true!); maybe it should get checked, I have placed some TODOs in there to get the reader's attention.

I also ran gen-modules > dist > uglify but I am not sure if I should have or if they are generated automatically.

@mkarajohn mkarajohn force-pushed the add_types branch 2 times, most recently from 9f9f99d to d510340 Compare December 21, 2023 04:43
@1cg
Copy link
Contributor

1cg commented Dec 22, 2023

Hi there. A couple of requests:

  • Can you undo the changes to /dist? I'll generate those when we do the next release
  • Do you know how to add a type checking task to the npm?

@mkarajohn
Copy link
Author

mkarajohn commented Dec 22, 2023

  • Can you undo the changes to /dist? I'll generate those when we do the next release

Done

  • Do you know how to add a type checking task to the npm?

We will have to install typescript as a developer dependency. We won't be writing the code in TS nor we'll be shipping compiled code (...unless you want to write the source in TS and ship compiled code?); it's only needed in order to parse our JSDoc definitions and validate the code against them. If you are ok with that, we can add something like this

"typecheck": "tsc --allowJs --checkJs --noEmit --target ES5 src/*.js"

What do you say?

@mkarajohn
Copy link
Author

mkarajohn commented Dec 22, 2023

For the record I am currently giving it a 2nd pass to clean up type errors that showed up with TS setup locally. I will be pushing a second commit during the day.

@mkarajohn
Copy link
Author

mkarajohn commented Dec 22, 2023

Ok, the prevalent issue is that there are lots of cases such as .nextSibling, which return either Node or null values and these values are then passed to functions that expect Element arguments (e.g. the function isSoftMatch clearly expects Elements (or null) as arguments, since internally it tries to access properties that only exist in Elements and not Nodes, but the function itself is called with the return value of something.nextSibling, for example, which can either be Node or null)

Maybe properties such as .nextElementSibling should be used instead?

@mkarajohn
Copy link
Author

mkarajohn commented Dec 22, 2023

Ok, a few comments:

  • I went ahead and set up the typescript compiler. It is just a dev dependency, it's irrelevant to the final user.
  • I fixed up a few more types based on the type errors that were thrown
  • Even though you seem to like to reassign values to function arguments, this creates friction when working with types, because your reassignment may be casting the value to a different type than the one defined for the argument (e.g. a stupid example: function(a: number) { a = 'foo'; } --> a is still considered a number, so the reassignment will throw a type error). You should prefer declaring new variables instead of reassigning arguments
  • I tried to only make changes to type definitions, and let you handle the type errors as you see fit. (only small change I made was to the mergeDefaults, createMorphContext and insertSiblings).
  • In general, in order to avoid type errors, prefer to use set/getAttribute instead of direct assignment.
  • You should decide how you want to handle the Node/Element issue. (possible solutions: allow for both in functions that now only accept one of them and make extra runtime checks to ensure type correctness, or, in places where you expect Element but pass Node use the corresponding attributes for getting an Element value (e.g. .nextElementSibling etc)
  • Overall, I did not touch the code itself, I left it up to you to decide how to approach type errors. (You can always choose to not type anything apart from the exported values of Idiomorph; this way end users get the types they need and you don't need to bother with types in the internals of the module 👀 )
  • In general the type errors point out instances where bugs could legit show up.

I am here to help with whatever you decide to do

@mkarajohn
Copy link
Author

@1cg

Not trying to be a nuisance, but is this still under consideration or should I stop thinking about it? 😂

It's fine if this won't go any further, I just need to have some kind of closure 😅

@1cg
Copy link
Contributor

1cg commented Dec 30, 2023 via email

@1cg
Copy link
Contributor

1cg commented Jan 8, 2024

Still interested in this, but htmx 2.0 is taking up all my time. I promise I will get to it by the end of the month though.

@mkarajohn
Copy link
Author

mkarajohn commented Jan 8, 2024 via email

@1cg
Copy link
Contributor

1cg commented Jan 29, 2024

going to look at this tonight

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.

2 participants