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

Test typescript types using tsd #158

Merged
merged 4 commits into from
May 30, 2021
Merged

Test typescript types using tsd #158

merged 4 commits into from
May 30, 2021

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented May 27, 2021

This does not yet have full coverage over all of the types in peg.d.ts;
I want to see if this approach seems viable to the typescript experts.

@hildjj
Copy link
Contributor Author

hildjj commented May 27, 2021

Note to future self: to generate the package-lock.json file with an older npm (to keep lockfileVersion: 1):

docker run --rm -it -v $PWD:/app --workdir /app node:12.22.1 npm install --package-lock-only

@hildjj hildjj added this to the 1.2 milestone May 28, 2021
@hildjj
Copy link
Contributor Author

hildjj commented May 28, 2021

Fixes #155

@hildjj hildjj mentioned this pull request May 28, 2021
3 tasks
Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems good, but I'm prefer to split one commit into several. In the current state it changes too much weakly connected things. I suggest to split it into:

  • update dependencies
  • change format of package-lock.json
  • switch jest from command-line switches to the config file
  • fix SyntaxError inheritance
  • add tsd and the stuff
  • update minified files (you can include this step into commit that changed them)

in any order which would build on each step

Comment on lines 41 to 48
switch (event.type) {
case "rule.enter":
expectType<peggy.LocationRange>(event.location);
break;
case "rule.match":
expectType<peggy.LocationRange>(event.location);
expectType<any>(event.result);
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"rule.fail" also has a location.

Suggested change
switch (event.type) {
case "rule.enter":
expectType<peggy.LocationRange>(event.location);
break;
case "rule.match":
expectType<peggy.LocationRange>(event.location);
expectType<any>(event.result);
break;
}
expectType<peggy.LocationRange>(event.location);
if (event.type === "rule.match") {
expectType<any>(event.result);
}

const p1 = peggy.generate(src, { output: "parser", grammarSource: "src" });
expectType<peggy.Parser>(p1);

const p2 = peggy.generate(src, { output: "source" });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why second test doesn't take a grammarSource?

Suggested change
const p2 = peggy.generate(src, { output: "source" });
const p2 = peggy.generate(src, { output: "source", grammarSource: "src" });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Willfix, with something not a string.

expectType<peggy.ast.Grammar>(grammar);

const visited: { [key: string]: number; } = {};
function add(typ:string): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function add(typ:string): void {
function add(typ: string): void {

Comment on lines +79 to +83
expectType<peggy.ast.Grammar>(node);
expectType<"grammar">(node.type);
expectType<peggy.LocationRange>(node.location);
expectType<peggy.ast.TopLevelInitializer|undefined>(node.topLevelInitializer);
expectType<peggy.ast.Initializer|undefined>(node.initializer);
expectType<peggy.ast.Rule[]>(node.rules);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is necessary to check types of node fields? Hasn't the whole node test checked them out yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm trying to achieve is that we'll notice if we break backward-compatibility in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

Comment on lines 114 to 115
expectType<peggy.LocationRange>(node.location);
expectType<peggy.LocationRange>(node.nameLocation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to check a name, if that even needed:

Suggested change
expectType<peggy.LocationRange>(node.location);
expectType<peggy.LocationRange>(node.nameLocation);
expectType<peggy.LocationRange>(node.location);
expectType<string>(node.name);
expectType<peggy.LocationRange>(node.nameLocation);

// @dts-jest:pass:snap
visit(grammar);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, that construction mean to run visit(grammar) and assert types with snapshot that was taken on the first run. However that snapshot should be committed and it seems it is not committed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mistake, left over from a previous attempt to use dts-jest. Will remove.

@hildjj
Copy link
Contributor Author

hildjj commented May 28, 2021

@Mingun I think i fixed everything, but I'm interested in your opinion about being able to detect backward-compat breaks.

@hildjj
Copy link
Contributor Author

hildjj commented May 28, 2021

@Mingun I think i fixed everything, but I'm interested in your opinion about being able to detect backward-compat breaks.

nm, just saw your comment. ok to merge?

@Mingun
Copy link
Member

Mingun commented May 29, 2021

LGTM. One minor note: is it necessary to generate declaration files? Automatically-generated files seems very useless because almost all types inferred as any. Besides, we already has our own file.

@hildjj
Copy link
Contributor Author

hildjj commented May 30, 2021

I agree that we don't need to generate .d.ts files. They will have more interesting things in them once we start documenting our interfaces jsdoc-style, but they're never going to generate something close enough to what we currently have to be backward-compatible.

@hildjj hildjj merged commit 48fd800 into peggyjs:main May 30, 2021
@hildjj hildjj deleted the test-types branch May 30, 2021 18:10
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.

2 participants