@NaridaL refactor(utils): set tsconfig strict=true for utils package and fix errors #1679

NaridaL · 2021-10-23T10:22:53Z

No description provided.

packages/utils/src/api.ts

bd82 · 2021-10-23T11:44:33Z

packages/chevrotain/src/parse/cst/cst_visitor.ts

@@ -160,8 +159,8 @@ export function validateRedundantMethods(
  for (const prop in visitorInstance) {
    if (
      isFunction(visitorInstance[prop]) &&
-      !contains(VALID_PROP_NAMES, prop) &&
-      !contains(ruleNames, prop)
+      !VALID_PROP_NAMES.includes(prop) &&


includes is not supported in IE11
and while I intend to drop support for legacy runtimes this has not been made official yet

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes#browser_compatibility

Yeah I guess I missed that, I looked for the engines field in the package.json.

As my other PRs had breaking changes too, we could drop IE11 support with 10? An alternative is to use the standardized functions such as .includes etc but use a polyfill for IE11 instead of using custom function everywhere.

This change also assumes the property is always a valid array, while contains (depending on how its implemented) could work on null/undefined values as well.

I'm fine with targeting more modern runtimes in version 10.0 and dropping IE11 support

I will answer this in the root PR chat

bd82 · 2021-10-23T12:33:50Z

I moved this to a discussion

bd82 · 2021-10-23T12:46:58Z

Moved to discussion #1681

bd82 · 2021-10-23T15:04:23Z

Perhaps this PR contains too many topics,
Maybe the replacing the custom utils functions topic should be handled in a separate PR.
and then we can more easily merge the strict type checks and api.d.ts improvements ?

…rrors This also created some errors in other packages due to variables now being typed correctly. These errors were also fixed. This includes a non-type fix in errors_public.ts. Also removed packArray function entirely as it was broken. BREAKING CHANGE: packArray function was removed from utils

NaridaL · 2021-10-23T16:06:01Z

Perhaps this PR contains too many topics, Maybe the replacing the custom utils functions topic should be handled in a separate PR. and then we can more easily merge the strict type checks and api.d.ts improvements ?

They're kind of related because I just replaced some methods instead of trying to fix their types. But I've reset this PR to the first commit, let's get this merged first (y). I assume you also consider changes to @chevrotain/utils API as breaking?

bd82 · 2021-10-23T21:44:28Z

I assume you also consider changes to @chevrotain/utils API as breaking?

Nope, while its technically a public package, the important API is the one specified in the types package and implemented in the chevrotain root package.

Feel free to break whatever you want in the utils package.
Although I will probably make an attempt to remove it completely soon(ish).

packages/chevrotain/src/scan/lexer.ts

packages/chevrotain/src/scan/lexer_public.ts

bd82 · 2021-10-24T17:15:07Z

packages/types/api.d.ts

@@ -1246,7 +1246,7 @@ export interface ILexerConfig {
   * This can be useful when wishing to indicate lexer errors in another manner
   * than simply throwing an error (for example in an online playground).
   */
-  deferDefinitionErrorsHandling?: boolean
+  deferDefinitionErrorsHandling: boolean


I don't get it, why are these properties that are definitively optional, marked as none optional?

Once the lexer is initialized, all the properties are set (by the user or defaults). Hence it makes sense to have all properties non-optional in the interface to prevent "can be undefined" errors anytime you access an option.

The parameter to the Lexer constructor is then changed to Partial so not all of them have to be explicitly passed.

I like the idea in this approach, but it won't "scale" if there is ever any property that is not optional.
I suggest the complement approach, internally transforming the Interface using Required and leaving the public API
with the "correct" typings and without requiring consumers to understand Mapped types in TypeScript

Yeah, that's the way to go

packages/utils/tsconfig.json

bd82 · 2021-10-24T17:29:40Z

packages/types/api.d.ts

@@ -2300,6 +2300,7 @@ export interface IProduction {

 export interface IProductionWithOccurrence extends IProduction {
  idx: number
+  maxLookahead?: number


Not all IProductionWithOccurrence can have maxLookahead.
Only those that involve Lookahead logic, e.g OR / MANY / OPTION,
But a Terminal / Non-Terminal would not...

hence why it is optional

In packages/chevrotain/src/parse/grammar/checks.ts 532 currProd.maxLookahead is accessed. currProd has type IProductionWithOccurrence, so it needs to have maxLookahead declared.

(It used to be IProduction, but currProd.idx is also accessed)

This is a public API so it should be more strict.
I'd prefer to only add this maxLookahead where it actually exists (e.g via an additional interface, or a property on each concrete type.

And if needed change/fix the internal code to deal with it, (even if in an ugly manner, e.g casting)

If the AST nodes are public, then it stands to reason that users may also need the maxLookahead property? On an interface this doesn't really mean much except "IF a instance has this property, then it must be of type number"?

If the AST nodes are public, then it stands to reason that users may also need the maxLookahead property?

To be honest, that is a great question which I did not fully consider.
Initially I was automatically in favor of exposing it.

But realistically I can't imagine any scenario in which a consumer would be interested in it.
The internal GAST is already for unique and advanced flows, e.g building your own syntax diagrams.
But would the maxLookAhead really matter for such scenarios? maybe it is just an implementation detail and should be removed from the public API.

packages/types/api.d.ts

bd82 · 2021-10-24T17:53:08Z

unfortunately only now realized the PR targeted the strict mode only on the utils package.

@NaridaL I truly appreciate your time investment in improving the code quality of this project.
Unfortunately I fear the wrong target was chosen ( utils package) as it is slated for removal attempt.

Partially due to issues raised in this PR...

Perhaps a more incremental approach would be to enable a single sub-strict setting in the root ts.config
and try to fix the related errors a small batch at a time hopefully there would be less than 880 😄

I just checked it and there seems to only be an handful of options related to strict so that approach may not suffice, some options display 8 errors (easy to fix) and others 300+

Maybe the long term approach to reach strict=true is actually the one you started, by applying strict on per sub-packages basis, but this approach would only become more relevant if and when Chevrotain will be split into more sub-packages (work in progress) and have far less code in the monolith legacy package (chevrotain).

Example, see: extract diagrams to a separate sub-package #1395

bd82 · 2021-10-24T17:54:45Z

I will close this PR, but will attempt to enable some sub-strict options on the root ts.config to at least stabilize the current status.

NaridaL · 2021-10-24T18:19:48Z

@bd82 While I did only enable strict in the utils package, this revealed a lot of errors/problems outside the utils package. Hence why most of the improvments are actually to files outside the utils package. This PR should therefore still be merged, even if you intend to remove the utils package.

bd82 · 2021-10-24T18:23:15Z

While I did only enable strict in the utils package, this revealed a lot of errors/problems outside the utils package. Hence why most of the improvments are actually to files outside the utils package. This PR should therefore still be merged, even if you intend to remove the utils package.

I see, so we will need to do a few review iterations because I don't understand some of the fixes and some seem incorrect (api.d.ts). I will try to review it fully tomororw

packages/chevrotain/src/scan/lexer.ts

bd82 · 2021-10-24T20:05:25Z

packages/chevrotain/src/scan/lexer.ts

@@ -1086,7 +1088,7 @@ export function buildLineBreakIssueMessage(

 function getCharCodes(charsOrCodes: (number | string)[]): number[] {
  const charCodes = map(charsOrCodes, (numOrString) => {
-    if (isString(numOrString) && numOrString.length > 0) {


getCharCodes is used on input from an end-user (the custom lineTerminatorCharacters).
Until now if a user passed an empty string it would be ignored, now it would add NaN to the result.
I don't know what effect this could have, and not sure I want to spend the time investigating.

Could we keep the extra length condition to ensure numOrString.charCodeAt(0) will return a real number?

The function as written before returned numbers or empty strings, which didn't match the function return signature. As far as I can see, none of the callers of getCharCodes seem to handle strings?

Yeah good catch 👍 it seems like a bug, so maybe returning NaN is better (after your refactor)

bd82 · 2021-10-24T20:20:57Z

packages/chevrotain/src/scan/lexer_public.ts

@@ -186,7 +184,7 @@ export class Lexer {
        }
      })

-      if (this.config.skipValidations === false) {


this === false is a personal style, maybe I just don't see the ! mark well 😄

https://stackoverflow.com/questions/11831881/if-boolean-false-vs-if-boolean

The problem in JavaScript this change actually introduces semantic difference because of automatic conversion to boolean.

Note that due to the automatic conversions more operations are made, so in hotspots (e.g tokenize method) I make sure to compare to exactly the right value (true/false/null/undefined) for performance reasons.

The 2nd problem is that there are ~70 other places in the code that perform === true/false so if we change style we should change it everywhere.

The 3rd problem is that we have enough topics in this PR already, lets avoid style questions as part of it.

OK. To me this reads as if this was done for the explicit semantic difference. But as skipValidations can actually only be true or false here it is confusing.

But as skipValidations can actually only be true or false here it is confusing.

That is a good point, skipValidations is provided by an end user, and can be anything, because TypeScript
is a design time tool only which is not even used by all (most?) consumers.

Well I'm assuming the passed values are valid according to TS definitions.

That is a very strong assumption when it a value typed in by an end user :)

bd82 · 2021-10-24T20:23:10Z

packages/chevrotain/src/scan/lexer_public.ts

@@ -262,7 +260,8 @@ export class Lexer {
              this.charCodeToPatternIdxToConfig[currModName] =
                currAnalyzeResult.charCodeToPatternIdxToConfig

-              this.emptyGroups = merge(
+              this.emptyGroups = Object.assign(


keep the merge so it would be easy for me to replace with 3rd party library and

otherwise I will likely miss this and won't replace when I introduce the new library

or use the assign from the utils instead of re-introducing the merge if its easier...

bd82 · 2021-10-24T20:42:44Z

packages/utils/src/api.ts

-export function contains<T>(arr: T[], item): boolean {
-  return find(arr, (currItem) => currItem === item) !== undefined ? true : false
+export function contains<T>(arr: T[], item: T): boolean {
+  return arr.includes(item)


includes is okay only because ES5.1 was deprecated earlier today...
There may also be the behavior difference if undefined is ever passed (the type system does not guarantee it would not 100%)

But I hope to delete this utils package completely, so it can be merged for now.

bd82 · 2021-10-24T20:52:44Z

Okay I finished reviewing, see the in-lined comments.

Things to note:

Public API quality is more important than internal code quality.
Lets avoid style questions in this PR
Improving the runtime quality (not type signatures) of the utils package may not be important if I will indeed remove it soon.

NaridaL · 2021-10-25T07:16:20Z

I have fixed your comments. If style questions are important, then they should probably be added to an eslint or similar to avoid discussions in PRs :-)

bd82 · 2021-10-25T09:31:50Z

I have fixed your comments. If style questions are important, then they should probably be added to an eslint or similar to avoid discussions in PRs

I agree, The more people contribute to this repository (on regular basis) the more useful a linting tool becomes.
While I use ESLint in many projects (particularly work related projects).

However, I found that when I am the "main" contributor for a project I end up wasting more time dealing with false positives and ESLint configurations than true linting issues, which is why it is currently not used here.

Generally speaking, when contributing to an open source project you often would simply align with the existing style in the project/file. and focus on the change you want to implement, and the fewer topics per PR (one is best) the better.
That makes it easier to review and much more likely to be merged quickly.

bd82 · 2021-10-25T20:38:26Z

I'm merging this and may do micro changes (e.g maxLookAhead) in a different commit

bd82 · 2021-10-25T20:38:48Z

Thanks @NaridaL 👍

bd82 · 2021-10-25T21:16:14Z

This is the micro change I've done:

4758be2

bd82 · 2021-10-25T21:22:02Z

and this will ensure strict===true in new sub-packages

fa735af

NaridaL commented Oct 23, 2021

View reviewed changes

packages/utils/src/api.ts Outdated Show resolved Hide resolved

bd82 reviewed Oct 23, 2021

View reviewed changes

NaridaL marked this pull request as draft October 23, 2021 11:47

NaridaL force-pushed the dev/utils-strict-mode branch 2 times, most recently from 778bf05 to 17f6442 Compare October 23, 2021 16:01

NaridaL force-pushed the dev/utils-strict-mode branch from 17f6442 to 37c63bd Compare October 23, 2021 16:02

NaridaL marked this pull request as ready for review October 23, 2021 16:06