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

Add autogenerated TypeScript definitions #50

Merged
merged 5 commits into from May 10, 2019

Conversation

Projects
None yet
3 participants
@RReverser
Copy link
Contributor

commented Mar 8, 2019

An alternative way would be to generate TypeScript in the first place, and then Babel could be replaced by just TypeScript which would both transpile down to ES2015 and generate valid definitions, but for now went with a less invasive change.

Fixes #39.

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Thanks for doing this! I confess I'm not very familiar with TypeScript, so this is going to be a somewhat incomplete review, but that's OK.

A couple points:

  • I notice you only define the Expression and Statement aggregate types. Is there a reason for this? The spec defines a bunch of others, which can be very useful - for example, if you accept a Script or Module, you can instead just take a Program.
  • Is it possible to test these? Even just testing that it was well-formed TypeScript would make me happy, though testing that the types match the implementations would be better still.
@bakkot

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

(Also, I don't think this fixes #49?)

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@bakkot

  1. Expression and Statement helpers simply mirror what checked generator is doing - it's also just defining these two as shorthands. Technically it doesn't matter which ones are defined, as these are just private helpers. UPD: Just noticed this wasn't the case and they were exported; fixed it.
  2. Yeah, it's possible to test but not sure what would be the best way to do so yet... Just try to construct some basic AST in a TypeScript test file?
  3. You're right, wrong issue number. I'll fix that.

BTW, regarding (2) - what do you think about the suggestion to replace Babel with TypeScript? That would kinda eliminate the need to test the generated definitions as they would be created by TS itself from the same source as executable code, and so would be guaranteed to be in sync.

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

GitHub doesn't seem to be respecting the // Generated by ... comment pattern for some reason. Can you address it with a linguist-generated in a .gitattributes file?

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Hmm, what do you mean by respecting it?

@michaelficarra

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@RReverser It's not auto-collapsed in the "files changed" tab. See this PR as an example.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Interesting, I didn't even know Github supports some manual annotations for that. I thought it just uses size as a heuristics.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

That said, it's probably better to review the generated code to in case something looks out of line.

RReverser added some commits Mar 8, 2019

Add autogenerated TypeScript definitions
An alternative way would be to generate TypeScript in the first place, and then Babel could be replaced by just TypeScript which would both transpile down to ES2015 *and* generate valid definitions, but for now went with a less invasive change.

Fixes #39.

@RReverser RReverser force-pushed the RReverser:dts branch from 376fcdf to d53dfc9 Mar 16, 2019

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

  • Rebased and fixed package.json conflicts.
  • Fixed issue number in the commit message and the PR description.
  • Marked entire gen folder as autogenerated in .gitattributes in a separate commit.

RReverser added some commits Mar 16, 2019

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Expression and Statement helpers simply mirror what checked generator is doing - it's also just defining these two as shorthands.

Well, yeah, but that was just so that the code was shorter. In this case I think it would be actually useful to define and export all of the named types, because I think it would be actually useful to be able to declare a variable of type Expression (or Program or whatever) even though you can only construct concrete types.

But I don't mind too much either way.

Yeah, it's possible to test but not sure what would be the best way to do so yet... Just try to construct some basic AST in a TypeScript test file?

Sure, that'd be fine by me.

what do you think about the suggestion to replace Babel with TypeScript

Strongly opposed, personally.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Strongly opposed, personally.

Fine by me. Although curious - why? If adding typecheck tests, you need need TypeScript as a dev-dependency anyway, and this way you would need only one of them instead of both Babel and TypeScript (since TypeScript can also transpile ES6 -> ES5 while typechecking).

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Mostly I don't want learning TypeScript to be a prerequisite to contributing to Shift, for myself or others. More generally I trust Babel's compiler more than I do TypeScript's.

(Though also maybe we should just drop Babel too - node 4 is EOL, node 6 is EOL next month.)

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Mostly I don't want learning TypeScript to be a prerequisite to contributing to Shift, for myself or others.

Hmm, but this shouldn't be the problem - TypeScript would be just a generated intermediate artifact, just like current .js + .d.ts, and not something you actually have to write by hand.

Moreover, you don't even have to generate actual TypeScript syntax if that's the concern. TypeScript compiler supports JSDoc too, so you can generate definitions from regular annotated JavaScript.

Anyway, I'll leave that outside of the scope of this PR, as mentioned in the description, I knew such change would be controversial :)

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Added basic test that ensures that constructors in basic.js correspond to type definitions.

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Hm. It'd be better not to modify the existing test files; I'd prefer to have a new file which tsc is run against. And, ideally, negative tests as well as positive - files which fail to typecheck, where that behavior is asserted.

I can push that up myself if you don't find the time in the next couple days.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Negative ones are a bit harder unfortunately, as they require either a separate tool like dtslint with own config and non-trivial installation time, or a bunch of tsc invocations which would likely end up with non-cross-platform error code checks.

As for your first point, I think it's better to check the real executable tests rather than invent something custom, since it better represents real-world usage and also makes it easier to notice discrepancies between runtime checks in checked.js and static ones in TypeScript. As currently seen in the diff, these places are clearly marked and colocated, and, in fact, already helped me find one discrepancy where typings were wrong!

@RReverser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@bakkot Any chance we could move on with current implementation for now? It works, it's tested, it's not very intrusive and it feels that any further improvements shouldn't be hard to do as separate PRs.

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

Sorry for not getting back to this in a timely way. I should be able to take another look in the next couple of days.

@bakkot

bakkot approved these changes May 10, 2019

Copy link
Contributor

left a comment

Still not thrilled about the testing strategy, but since this has now been open for two months and is otherwise good I'm going to merge it as-is and we can revisit the tests later if we want.

@bakkot bakkot merged commit 9671126 into shapesecurity:es2017 May 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bakkot added a commit that referenced this pull request May 10, 2019

bakkot added a commit that referenced this pull request May 10, 2019

@RReverser RReverser deleted the RReverser:dts branch May 10, 2019

This was referenced May 10, 2019

michaelficarra added a commit that referenced this pull request May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.