Add type checking and fix issues

[Buckwich]

Closes #39

Proposed Changes

• add type checking with a not fully strict config (also added to CI)
• address type errors by adjusting jsDoc or casting
• actual implementation changes:
  • https://github.com/bpmn-io/feel-editor/blob/39_check-types/src/autocompletion/index.js#L25-L26: use completeFromList to convert snippet Completion[] to CompletionSource[]
  • https://github.com/bpmn-io/feel-editor/blob/39_check-types/src/autocompletion/pathExpression.js#L23-L24: always return null instead of undefined in pathExpressionCompletion

Try out

install and try the new script check-types

npm run check-types

currently no errors should be found

try to adjust jsdoc params in a ts capable editor, you should get in editor errors

if you prefer cli only just run the script again to see the errors

Checklist

To ensure you provided everything we need to look at your PR:

• Brief textual description of the changes present
• Visual demo attached
• Steps to try out present, i.e. using the @bpmn-io/sr tool
• Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

[Buckwich]

@bpmn-io/modeling-dev Do you like 2 tsconfigs (one for test, one for source, +helpers) or one unified as in camunda/camunda-modeler#5186 more?

[barmac]

Any idea of what it means that isList is "optional"?

[Buckwich]

not quite. the tests are using both boolean values and 'optional'. I didn't want to adjust the implementation (for now) and just fix the docs to reflect the implementation (unless its obviously broken).

optional is checked here: https://github.com/bpmn-io/feel-editor/blob/39_check-types/src/autocompletion/pathExpression.js#L43

[barmac]

Perhaps the answer is in the variable resolver. I will look for an answer in our codebase.

[barmac]

Indeed it's coming from variable resolver, and is supposed to handle cases where a variable is created once as a list, and once as a non-list: https://github.com/bpmn-io/variable-resolver/blob/e69f839538a8368024dfcbfcef7e56e64b53713a/lib/base/VariableResolver.js#L346
So essentially variable: T[] | U

[barmac]

Meaning: The variable can be a list, but does not have to.

[barmac]

@bpmn-io/modeling-dev Do you like 2 tsconfigs (one for test, one for source, +helpers) or one unified as in camunda/camunda-modeler#5186 more?

Do we even want to typecheck the tests? What is the benefit?

[Buckwich]

Do we even want to typecheck the tests? What is the benefit?

we don't have to typecheck them, but having a tsconfig for tests helps with autocompletion and intellisense. By having two separate configs we could adjust the CI to only enforce one, or check tests less strict. even though I would like to check everywhere with the same config.

Checking tests as well helps validating additional constraints, ie. if I would have just checked src one might think the correct type for isList is string. But by also checking test sources I realized both boolean and 'optional' are required. having the type check saved me from going really deep into the code, or at least pointed me to the right place

[barmac]

Thanks for the clarification. Can we then have 2 tsconfig files: one for source code, one for tests? Currently, I see 4 files listed.

[Buckwich]

can we then have 2 tsconfig files: one for source code, one for tests? Currently, I see 4 files

the base helper is needed to share configs between the src and test. the main tsconfig is an easier entry point for ides and commands, but could be adjusted to just have two tsc commands. Cf b74e767 with some duplicated configs

Note: this only works if test and src are separate, eg in camunda-modeler this wouldn't work as we have __test__ inside src (app/client)

[barmac]

I've just pushed 1c40800 which I think is the simplest config we can maintain. Two separate config files but the source one includes most of the configuration. WDYT?

[barmac]

✅ from me. Great work

[Buckwich]

the problem with 1c40800 is that vscode then doesn't work in test (eg no intellisense, no type checking), only the cli works

[Buckwich]

i applied your idea of extending source in fa846d8

[barmac]

OK but it works with ed69c6d

[Buckwich]

yeah, its the same. then i'm fine with it

[barmac]

i applied your idea of extending source in fa846d8

I missed that, but generally I think tsconfig should be placed in the root unless it's a test-specific config.

[Buckwich]

i'll go ahead and squash the commits and force push

[Buckwich]

@barmac are you happy with the config now, I'm planning to apply the same also to other projects (where we have separate src/test folders)

[barmac]

Absolutely! Great work