Skip to content

Add JSDoc types and definitions#25

Merged
1cg merged 5 commits into
bigskysoftware:mainfrom
mkarajohn:add_types
Oct 12, 2024
Merged

Add JSDoc types and definitions#25
1cg merged 5 commits into
bigskysoftware:mainfrom
mkarajohn:add_types

Conversation

@mkarajohn

@mkarajohn mkarajohn commented Dec 21, 2023

Copy link
Copy Markdown
Contributor

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

1cg commented Dec 22, 2023

Copy link
Copy Markdown
Contributor

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

mkarajohn commented Dec 22, 2023

Copy link
Copy Markdown
Contributor Author
  • 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

mkarajohn commented Dec 22, 2023

Copy link
Copy Markdown
Contributor Author

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

mkarajohn commented Dec 22, 2023

Copy link
Copy Markdown
Contributor Author

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

mkarajohn commented Dec 22, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor 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

1cg commented Dec 30, 2023 via email

Copy link
Copy Markdown
Contributor

@1cg

1cg commented Jan 8, 2024

Copy link
Copy Markdown
Contributor

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

mkarajohn commented Jan 8, 2024 via email

Copy link
Copy Markdown
Contributor Author

@1cg

1cg commented Jan 29, 2024

Copy link
Copy Markdown
Contributor

going to look at this tonight

@1cg

1cg commented Oct 12, 2024

Copy link
Copy Markdown
Contributor

lmao gonna finally merge this

@1cg
1cg merged commit 5da195e into bigskysoftware:main Oct 12, 2024
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