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

Avoid using Array.prototype.find #582

Closed
tinydylan opened this issue Apr 22, 2020 · 11 comments
Closed

Avoid using Array.prototype.find #582

tinydylan opened this issue Apr 22, 2020 · 11 comments
Labels
✔️ Feature Accepted first-timers-only good first issue easy issue to start with - contributions welcome help wanted contributions welcome

Comments

@tinydylan
Copy link
Contributor

💡 Idea

constantFrom uses Array.prototype.find, which is not supported on IE11.

Motivation

TinyMCE is a popular open source product and we have switched from jsc to fast-check in the last year. We still support IE11.

@tinydylan
Copy link
Contributor Author

We hit this issue when using the webUrl arbitrary.

@dubzzz
Copy link
Owner

dubzzz commented Apr 22, 2020

Good point, I think we can switch to Array.prototype.indexOf while keeping the same behaviour.

Marking the issue as 'good first issue'

@dubzzz dubzzz added first-timers-only good first issue easy issue to start with - contributions welcome help wanted contributions welcome ✔️ Feature Accepted labels Apr 22, 2020
@tinydylan
Copy link
Contributor Author

Array.prototype.find takes a predicate, whereas Array.prototype.indexOf takes a normal value.

Would you be ok with me adding something like this:

export function findOrNull<T> (ts: ArrayLike<T>, f: (t: T) => boolean): T | null {
  for (let i = 0; i < ts.length; i++) {
    const t = ts[i];
    if (f(t)) return t;
  }
  return null;
} 

in a file like src/check/arbitrary/helpers/ArrayHelper.ts ?

@tinydylan
Copy link
Contributor Author

PR is up: #583

Happy to adjust the code whatever the project expects.

@dubzzz
Copy link
Owner

dubzzz commented Apr 25, 2020

Thanks a lot for the contribution. I started a release of 1.24.2 that will contain your fix. I may backport the fix on older versions if needed (but as I follow semver, bumping to 1.24.2 should not be problematic)

@dubzzz
Copy link
Owner

dubzzz commented Apr 28, 2020

TinyMCE is a popular open source product and we have switched from jsc to fast-check in the last year. We still support IE11.

@tinydylan Whaouu great news :) Please let me know if you miss some features that were provided by jsc. Or what you liked more or less in fast-check in general

@tinydylan
Copy link
Contributor Author

tinydylan commented Apr 28, 2020

:)

At risk of sending this issue off-topic:

At Tiny, our engineering team uses functional programming heavily. We write in Scala, Haskell, TypeScript and ReasonML, and use FP stuff all through it. We're no strangers to property testing - we use QuickCheck and ScalaCheck, and in our TypeScript code we were using Jsc.

TinyMCE was ported from JS to TS over the last few years and we've been gradually adding types all through it. Unfortunately, Jsc's TS bindings were a bit lacking, so we switched to fast-check. We're starting to move more of TinyMCE to ReasonML and one of our engineers has made ReasonML bindings for fast-check.

I'm a huge fan of fast-check, to be honest. fast-check does pretty much everything I expect, the way I expect it. You've really done a great job.

The IE support has been the biggest issue so far - hence the PR. We also hit some issues because fast-check uses Symbol - again, not supported by IE. We've polyfilled that in our testing tooling for now, but will probably log a ticket for that soon.

I logged a ticket about non-empty lists and non-empty strings a while ago - and I was happy with the answer. I find with a lot of these property test tools that it helps to have quite a wide range of generators built-in. Non empty strings, ascii strings, uppercase ascii strings, alphanumeric strings, hex strings, non-empty hex strings, non-empty uppercase hex strings, number-except-for-NaN, positive ints, negative ints, natural numbers, rationals, domain names, tcp ports, html tags. I like there to be lots of generators.

A liftA2/liftA3 etc could be handy - maybe something like fp-ts's pipe or fp-ts-contrib's attempt at do notation. I really wish we could use proper do notation in TS - that would make composing generators much easier.

Anyway - I'll log separate tickets for these if they become important. For now - I think we can close this ticket, and on behalf of Tiny, we thank you and your contributors for helping us crush edge cases in TinyMCE.

@tinydylan
Copy link
Contributor Author

BTW - I don't think I've seen anything in jsc that is missing from fast-check. We still have some code to port - mostly tests that generate complex HTML.

@dubzzz
Copy link
Owner

dubzzz commented May 25, 2020

Is it working fine now? If so can we close the issue?

@tinydylan
Copy link
Contributor Author

Testing now. Will get back to you soon.

@tinydylan
Copy link
Contributor Author

Yep. Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✔️ Feature Accepted first-timers-only good first issue easy issue to start with - contributions welcome help wanted contributions welcome
Projects
None yet
Development

No branches or pull requests

2 participants