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

support accepting a function in the errorMsg option #126

Merged
merged 6 commits into from
May 5, 2020

Conversation

peburrows
Copy link
Contributor

This allows the error message to be evaluated at the time of failure (instead of at call time), so it can potentially include details that may differ from when waitUntil was first called. Specifically, this lets you include more detailed information in the error message about what exactly failed.

This allows the error message to be evaluated at the time of failure,
and potentially include details that may differ from when
`waitUntil` was first called.
@NoriSte
Copy link
Owner

NoriSte commented Apr 30, 2020

Hi @peburrows
Thanks for your contribution! Well, it sounds good. I think that:

  • the errorMsg callback should receive an object containing { result, options } (please note that the options I refer to are the originalOptions in the source code) what do you think? This gives all the freedom to the callback
  • I think that a typeof options.errorMsg=== 'function' would be enough and more readable in this case
  • are you able to update the TS definitions?

@NoriSte
Copy link
Owner

NoriSte commented Apr 30, 2020

@all-contributors please add @peburrows for ideas, code and test

@allcontributors
Copy link
Contributor

@NoriSte

I've put up a pull request to add @peburrows! 🎉

@peburrows
Copy link
Contributor Author

the errorMsg callback should receive an object containing { result, options } (please note that the options I refer to are the originalOptions in the source code) what do you think? This gives all the freedom to the callback

Done!

I think that a typeof options.errorMsg=== 'function' would be enough and more readable in this case

I went with options.errorMsg instanceof Function since this was already used elsewhere in the code.

are you able to update the TS definitions?

I've updated the TS definitions in index.d.ts.

I also noticed cypress/types/plugin.spec.ts, but I'm not sure I understand what that file is actually used for. I went ahead and updated it, but I can't say I'm exactly clear on whether that was necessary or if what I did was correct there.

@NoriSte
Copy link
Owner

NoriSte commented Apr 30, 2020

Thank you @peburrows I'm going to review it in the next days 😊

@NoriSte
Copy link
Owner

NoriSte commented May 4, 2020

Hi @peburrows
Could you please give me write access to your own fork? I'd like to push some additional commits where:

  • I've updated the Readme
  • I've updated the TS definitions
  • I've updated the cypress/types/plugin.spec.ts file

@NoriSte
Copy link
Owner

NoriSte commented May 4, 2020

I also noticed cypress/types/plugin.spec.ts, but I'm not sure I understand what that file is actually used for. I went ahead and updated it, but I can't say I'm exactly clear on whether that was necessary or if what I did was correct there.

This is the easiest way to have TypeScript validation without writing the plugin in TypeScript (see #18), what I mean:

  • the plugin is written in JavaScript
  • the plugin has the index.d.ts type definition file to add TypeScript validation
  • but: who can check that the TS definition is correct? What if I write the JS function receiving a booolean and I write the wrong TS definition reporting that the function receives a string? Only TS could tell that...

The solution is:

  • adding a plugin.spec.ts file that (fakely) consumes the waitUntil function the way we expect it to work
  • adding a dedicated script that compiles the plugin.spec.ts (take a look at the package.json scripts)
  • launch the typescript script before the tests
  • if the TS type definitions are wrong or the way the plugin.spec.ts consumes the plugin is wrong, the test script fails

You can check it directly in VSCode what happens when the plugin is consumed uncorrectly
Screenshot 2020-05-04 at 08 45 53

Let me know if it's clear 😊

@peburrows
Copy link
Contributor Author

peburrows commented May 4, 2020

Could you please give me write access to your own fork?

I just invited you as a collaborator, so you should have write access.

@NoriSte
Copy link
Owner

NoriSte commented May 4, 2020

Thanks, take a look at my commit and let me know if they work for you 😉

@peburrows
Copy link
Contributor Author

Yep, looks great. Thanks!

@NoriSte
Copy link
Owner

NoriSte commented May 5, 2020

Perfect, I'm going to merge it 😊

Was the explanation for the cypress/types/plugin.spec.ts file clear?

@NoriSte NoriSte merged commit 5e13105 into NoriSte:master May 5, 2020
@NoriSte
Copy link
Owner

NoriSte commented May 5, 2020

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants