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

Remove any types and implement proper type assertions #16

Closed
wants to merge 4 commits into from

Conversation

AMoldskred
Copy link

@AMoldskred AMoldskred commented Oct 4, 2020

In this PR I have started to remove any-types by creating proper type-asssertions.
I have also moved some conditionals to make it both easier to read and for typescript to easier infer the correct type.

It is not a complete fix, but it should be a good start.
Before merging with master I would try to pull locally and test that it doesn't break anything.
I couldn't figure out how to use the correct ts-lint and how to run it. But I am fairly certain that it should be all good

#14

@thepassle
Copy link
Member

This is really great! Very valuable help here, I appreciate it a lot 🙂

It does seem like theres some breakage regarding the types however:
image

Could you take a look? You can confirm the types are working by, from the package root:

yarn install
cd packages/custom-elements-json-core
npm run tsc:watch

@AMoldskred
Copy link
Author

Will do!

@AMoldskred
Copy link
Author

@thepassle I'm not going to lie. There needs some serious work done to make typing to work properly in this project.
I have done what I can for now to fix the typing, however there is one place where I had to force any type in src/ast/handleEvents (There was no logical way that I could see that would infer the correct type).

@AMoldskred AMoldskred requested a review from thepassle October 8, 2020 09:41
@AMoldskred
Copy link
Author

@thepassle It would be really great if you could tag this PR with hacktoberfest-accepted so that I could get this as a PR towards my 4 in hacktoberfest😉

@thepassle
Copy link
Member

@thepassle It would be really great if you could tag this PR with hacktoberfest-accepted so that I could get this as a PR towards my 4 in hacktoberfest😉

Sorry for the late reply, I've had some IRL stuff to attend to this past week, and I've also been considering the approach of this project, and potentially just moving it over to JS with JSDoc, but I'm still unsure on what to do.

Does adding a hacktoberfest-accepted immediately count towards your hacktober fest progress? Even if this doesn't get merged? If so, I'll happily add it so you can complete your hacktoberfest goal.

@AMoldskred
Copy link
Author

Thank you @thepassle ! The PR started counting as soon as you added the label. I wish you best of luck with your project!

@AMoldskred AMoldskred closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants