electerious / basicScroll

Standalone parallax scrolling for mobile and desktop with CSS variables.
https://basicscroll.electerious.com
MIT License
3.63k stars 148 forks source link

Feature/ts compatible jsdoc #37

Closed jantimon closed 5 years ago

jantimon commented 5 years ago

This pull request improves the jsdoc comments to be compatible with typescript.

Therefore typescript is able to tell if you set an option which is not allowed:

basicscroll-wrong

It will also let you know if you forgot a value (e.g. to):

basicscroll_-_incomplete

And it will add autocomplete only for valid values:

basicscroll-autocomplete basicscroll_-_autocomplete_2
jantimon commented 5 years ago

I also merged both arguments of mapDirectToProperty as it used direct and properties.direct which are identical: https://github.com/electerious/basicScroll/pull/37/commits/64597ebae6a0b72254f92ca79f9e970f6fe8ddfd

jantimon commented 5 years ago

It was also possible to use Object.assign for default values to reduce the code size and to keep a consistent type for faster execution:

https://github.com/electerious/basicScroll/pull/37/commits/4dd192ca0ea7e3e17aae1449068752d8f3822afe

jantimon commented 5 years ago

Please let me know if you need further help for this pull request :)

electerious commented 5 years ago

Hey @jantimon, thanks for your work!

I'm sorry to say that I don't want to maintain all those typedef stuff and it would take a bit of fun out of basicScroll if I had to. I will therefore stay with the current JSDoc implementation which is consistent with my other projects.

It's a bit more than I thought. To be honest, I just expected something like this: https://github.com/electerious/basicScroll/pull/37/commits/901a683826629bc7fc3eca47a030de6fe8726eb4

I was just about to say no (see my original answer above), but I guess I need to learn a bit more about the JSDoc changes and typedef stuff first. Maybe it's not as annoying as it looks.

Can you recommend a good article about how to make JSDoc TS conform? I will look further into this when I have time. It currently has no priority for me.


@param {function} fn
@param {number} duration

It would have been better to not include the Object.assign and mapDirectToProperty in this PR. They aren't related and making it harder to review.

jantimon commented 5 years ago

Hey @electerious cool :)

In the end typings like jsdoc or typescript only add some information about your variables so that the computer (and hopefully also humans) can understand them.

About all those jsdoc features:

Typescript introduced jsdoc support and therefore modern editors like Visual Studio Code are able to support you with those typings:

https://github.com/Microsoft/TypeScript/wiki/JsDoc-support-in-JavaScript

But those type annotations inside jsdoc were not an invented by Typescript in fact Microsoft reused work from Googles Closure compiler:

https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler

In these typing systems new String('foo') would be of type String but "hello world" would be of type string. That's why I had to change some of them.

The reason why I changed Object.assign and mapDirectToProperty is that it is harder to explain the computer that the type of a variable changed. So I tried to reduce the amounts of different types used in your code so it would be easier to understand for the computer what values should be inside it. And this also makes it easier for humans to understand too :)

jantimon commented 5 years ago

Can I do anything to help you merging this?

We found some bugs and would love to provide some bugfixes but thats way easier on typed code.

electerious commented 5 years ago

Thanks again for the work and the answers to my questions, but this changes won't make it into the project.

I'm sorry to say that I don't want to maintain all those typedef stuff and it would take a bit of fun out of basicScroll if I had to. I will therefore stay with the current JSDoc implementation which is consistent with my other projects.