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

Update .d.ts definitions #143

Merged
merged 11 commits into from
May 22, 2021
Merged

Update .d.ts definitions #143

merged 11 commits into from
May 22, 2021

Conversation

Mingun
Copy link
Member

@Mingun Mingun commented May 3, 2021

Working on the new way of reporting errors I realized, that peg.d.ts is not true. It contains several definitions that was never exist and has a lack of types, that I plan update in my new PR.

So I decided to fix definitions first. This lead to some uncompatible changes in the GrammarError definition, but, judging that fact that it was incorrect, nobody could use it. So we can do that uncompatible change, because that is a bug fix

@hildjj
Copy link
Contributor

hildjj commented May 3, 2021

I'm happy to take this as a stopgap, but we have a working typescript pass in the build now, which generates .d.ts files then throws them away. I'd like to turn on type checking, then use the output of typescript if we can get it compatible-enough with what we've had in the past.

@hildjj
Copy link
Contributor

hildjj commented May 3, 2021

We also need a way to test this file, at least for syntax. Maybe we should add a .ts file to the examples directory?

@Mingun
Copy link
Member Author

Mingun commented May 3, 2021

I doubt that automatic inferring could infer that:

peggy/lib/peg.d.ts

Lines 478 to 497 in f5fca4e

interface Visitor<F extends NodeTypes> {
/**
* Runs visitor function registered for the specified node type.
* Returns value from the visitor function for the node.
*
* @param node Reference to the AST node
* @param args Extra arguments that will be forwarded to the corresponding
* visitor function, associated with `T`
*
* @template {T} Type of the AST node
*/
<T extends keyof NodeTypes>(node: ast.Node<T>, ...args: any[]): ReturnType<F[T]>;
}
/**
* Creates visitor object for traversing the AST.
*
* @param functions Object with visitor functions
*/
function build<F extends NodeTypes>(functions: F): Visitor<F>;

That code proves that you can do only that things:

let visit = visitor.build({
  simple_and() { return 1;  },
  simple_not() { return ""; },

  // no error, but this is because that method can be used by other methods
  not_exist1() { return false; },
});
let res1 = visit({ type: "simple_and" });// res1 type is a number - inferred from "simple_and"
let res2 = visit({ type: "simple_not" });// res2 type is a string - inferred from "simple_not"
let res3 = visit({ type: "not_exist2" });// error: unknown node type

Simplified example on typescript playground (strange, but it reports errors that my tsc (version=4.2.4) do not complained. Easy fixable, although)


I performing tests by changing tsconfig.json and by checking what SonarLint VSCode plugin shows me as types of imported peggy library (I imagine that I'm write a plugin for peggy):

 tsconfig.json | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tsconfig.json b/tsconfig.json
index fe8a4420..3f17e71e 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -4,7 +4,7 @@
     "module": "commonjs",                           /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */
     // "lib": [],                                   /* Specify library files to be included in the compilation. */
     "allowJs": true,                                /* Allow javascript files to be compiled. */
-    "checkJs": false,                               /* Report errors in .js files. */
+    "checkJs": true,                                /* Report errors in .js files. */
     // "jsx": "preserve",                           /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
     "declaration": true,                            /* Generates corresponding '.d.ts' file. */
     "declarationMap": false,                         /* Generates a sourcemap for each corresponding '.d.ts' file. */
@@ -14,8 +14,8 @@
     "removeComments": true,                         /* Do not emit comments to output. */
 
   /* over time we will enable these */
-    // "strict": true,                              /* Enable all strict type-checking options. */
-    // "noImplicitAny": true,                       /* Raise error on expressions and declarations with an implied 'any' type. */
+    "strict": true,                                 /* Enable all strict type-checking options. */
+    "noImplicitAny": false,                         /* Raise error on expressions and declarations with an implied 'any' type. */
     // "strictNullChecks": true,                    /* Enable strict null checks. */
     // "strictFunctionTypes": true,                 /* Enable strict checking of function types. */
     // "strictBindCallApply": true,                 /* Enable strict 'bind', 'call', and 'apply' methods on functions. */
@@ -34,5 +34,5 @@
     "skipLibCheck": true,                           /* Skip type checking of declaration files. */
     "forceConsistentCasingInFileNames": true        /* Disallow inconsistently-cased references to the same file. */
   },
-  "include": ["./lib/**/*.js", "./test/**/*.spec.js", "./benchmark/*.js"]
+  "include": ["./lib/**/*.js"]
 }

Making tests will be a good idea, but I still have no idea where to start. Opinions?

@phpnode
Copy link
Contributor

phpnode commented May 3, 2021

@hildjj
Copy link
Contributor

hildjj commented May 20, 2021

Is this next? or #144?

@Mingun
Copy link
Member Author

Mingun commented May 21, 2021

I think that this one is better to merge first

@Mingun
Copy link
Member Author

Mingun commented May 21, 2021

I've rebased, fix one minor thing from the previous PR (the first commit) so final review is expected.

@hildjj hildjj requested a review from phpnode May 21, 2021 19:26
@hildjj
Copy link
Contributor

hildjj commented May 21, 2021

I'm adding a few tests using dts-jest that can land after this.

[edit: removed issue i thought I was having, but wasn't actually]

Copy link
Contributor

@phpnode phpnode left a comment

Choose a reason for hiding this comment

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

LGTM

lib/peg.d.ts Outdated
@@ -1,42 +1,224 @@
// Based on PEG.js Type Definitions by: vvakame <https://github.com/vvakame>, Tobias Kahlert <https://github.com/SrTobi>, C.J. Bell <https://github.com/siegebell>

declare namespace PEG {
function parse(input: string): any;
/** Interface's that describe the abstract syntax tree used by Peggy. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Interface's that describe the abstract syntax tree used by Peggy. */
/** Interfaces that describe the abstract syntax tree used by Peggy. */

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@hildjj hildjj merged commit 58d7232 into peggyjs:main May 22, 2021
@Mingun Mingun deleted the ts-definitions branch May 22, 2021 18:14
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