Skip to content

Remove any types and implement proper type assertions#16

Closed
AMoldskred wants to merge 4 commits into
open-wc:masterfrom
AMoldskred:fix/remove-any-types
Closed

Remove any types and implement proper type assertions#16
AMoldskred wants to merge 4 commits into
open-wc:masterfrom
AMoldskred:fix/remove-any-types

Conversation

@AMoldskred
Copy link
Copy Markdown

@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

Comment thread packages/custom-elements-json-core/src/utils/index.ts
@thepassle
Copy link
Copy Markdown
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
Copy Markdown
Author

Will do!

@AMoldskred
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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