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

IParameterTypeDefinition regexp #1702

Closed
yasserf opened this issue Jun 22, 2021 · 6 comments · Fixed by #1733
Closed

IParameterTypeDefinition regexp #1702

yasserf opened this issue Jun 22, 2021 · 6 comments · Fixed by #1733
Labels
🐛 bug Defect / Bug good first issue Good for newcomers ✅ accepted The core team has agreed that it is a good idea to fix this

Comments

@yasserf
Copy link

yasserf commented Jun 22, 2021

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @cucumber/[email protected] for the project I'm working on.

The IParameterTypeDefinition in code supported both RegExp and arrays of them.

Here is the diff that solved my problem:

diff --git a/node_modules/@cucumber/cucumber/lib/support_code_library_builder/types.d.ts b/node_modules/@cucumber/cucumber/lib/support_code_library_builder/types.d.ts
index 27e0f51..13b4fbe 100644
--- a/node_modules/@cucumber/cucumber/lib/support_code_library_builder/types.d.ts
+++ b/node_modules/@cucumber/cucumber/lib/support_code_library_builder/types.d.ts
@@ -39,7 +39,7 @@ export interface IDefineTestRunHookOptions {
 }
 export interface IParameterTypeDefinition<T> {
     name: string;
-    regexp: RegExp;
+    regexp: RegExp | RegExp[];
     transformer: (...match: string[]) => T;
     useForSnippets?: boolean;
     preferForRegexpMatch?: boolean;

This issue body was partially generated by patch-package.

@aurelien-reeves
Copy link
Contributor

Hi :)

Thanks for the report.

Could you please give more details regarding the original issue you were facing?

Did you consider creating a pull request with your changes?

@aurelien-reeves aurelien-reeves added 🍼 incomplete Blocked until more information is provided 🐛 bug Defect / Bug labels Jun 25, 2021
@aslakhellesoy
Copy link
Contributor

The IParameterTypeDefinition interface in Cucumber.js is a wrapper around this constructor signature in the cucumber-expressions library:

https://github.com/cucumber/common/blob/5391796cb49ebc93b83325fcce96af2f59e9a201/cucumber-expressions/javascript/src/ParameterType.ts#L44

I think an even better fix would be to adopt the same type:

regexp: readonly RegExp[] | readonly string[] | RegExp | string

Note that the cucumber-expressions library uses the plural regexps while Cucumber.js uses the singular regexp. It's unfortunate that they are different, but for backwards compatibility reasons I think we should keep regexp.

@yasserf
Copy link
Author

yasserf commented Jun 25, 2021

The issue i faced was when defining custom parameter types:

For example:

defineParameterType({
  regexp: [/object using (.+)$/, /using object (.+)$/],
  transformer: function (this: World, not_sure: string, name: string) {
    ... does something ...
  },
  useForSnippets: true,
  preferForRegexpMatch: true,
  name: 'object',
})

note: the above example syntax makes no real sense, it's for a POC which I refactored a few times and so don't actually need both, but the library supports it so the typescript patch stayed

I raised the issue because I had a patch via patch-package and it offered to do it via a template, so was pretty quick 🙈

happy to raise a PR in the next few days if noone beats me to it!

@davidjgoss
Copy link
Contributor

@yasserf do you mean you wanted to use an array of regexps but it yielded a TypeScript compilation error (even though it would work at runtime)?

@yasserf
Copy link
Author

yasserf commented Jun 25, 2021 via email

@davidjgoss
Copy link
Contributor

Thanks for confirming @yasserf.

I've pushed a test for the TypeScript typing to this branch, which fails as of now and should pass (along with all existing change) once the change @aslakhellesoy suggests is made.

If you or anyone interested would like to do a PR for this, that would be great.

@davidjgoss davidjgoss added ✅ accepted The core team has agreed that it is a good idea to fix this good first issue Good for newcomers and removed 🍼 incomplete Blocked until more information is provided labels Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug good first issue Good for newcomers ✅ accepted The core team has agreed that it is a good idea to fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants