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

feat: type checking & type generation #41

Merged
merged 6 commits into from
Oct 10, 2020
Merged

feat: type checking & type generation #41

merged 6 commits into from
Oct 10, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Oct 8, 2020

image

Screen Shot 2020-10-09 at 4 45 57 PM

  • Adds typedef generation to a build script

@rvagg
Copy link
Member

rvagg commented Oct 9, 2020

What is the type checking actually doing? asserting that type definitions exist? what is the extent to which it can actually "check" these things? were the additions you've made here to the inline type definitions because the type checking was failing and needed more information?

@Gozala
Copy link
Contributor Author

Gozala commented Oct 9, 2020

What is the type checking actually doing? asserting that type definitions exist? what is the extent to which it can actually "check" these things?

No it's full type check, it will run typescript on the source but with JSDoc annotations instead of .ts files.

were the additions you've made here to the inline type definitions because the type checking was failing and needed more information?

I have explicitly set things up so types won't be implicitly inferred as any (that's also what generated types would have). That makes check report places where things are inferred as any and added annotations were to address those anys.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 9, 2020

As an aside we're doing this for js-ipfs as well and other libraries in the ecosystem as community had been asking for this. Not having something like this in IPLD would be pain for js-ipfs because we'd need to have to make some typedefs to fill the gaps there.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 9, 2020

Here is an example of an error that no existing tests or linter seems to catch, but is reported by type-checker. #43

P.S.: Line numbers don't seem to be picked up properly, but I'm trying to fix that

.github/workflows/mikeals-workflow.yml Outdated Show resolved Hide resolved
@mikeal
Copy link
Contributor

mikeal commented Oct 9, 2020

Looks good for the most part but I do want to cut the full type check from the default workflow for contributors. More specifics in my comment on the GitHub Action change.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 9, 2020

Looks good for the most part but I do want to cut the full type check from the default workflow for contributors. More specifics in my comment on the GitHub Action change.

Can we make them warning or something instead of full brown errors, so thy still show and contributors can choose whether to address them or not

@Gozala Gozala requested a review from mikeal October 9, 2020 20:59
@Gozala
Copy link
Contributor Author

Gozala commented Oct 9, 2020

@mikeal could you please take another look and merge it if it looks good ? Once it's inn I'll try it out with dag-service and verify that all the types are picked up as they should.

@Gozala Gozala merged commit 9cdcb3b into master Oct 10, 2020
@rvagg
Copy link
Member

rvagg commented Oct 12, 2020

Yeah thanks for the explanation @Gozala. I'm asking because I'm trying to figure out how much of this I (a) need to grok before I can get benefit from it and (b) can copy into my own projects while not having to fully down the rabbit hole. Like with Mikeal's GitHub Action and the set of npm scripts we've now collectively sort of agreed are useful to copy and paste to get the full test, lint, build, esm, cjs, stuff. Without having to implement a full aegir system, what are the basic patterns we can easily copypasta across our projects, and tweak in each instance as needed.

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.

3 participants