Is the TypeScript type definition wrong?

[SamVerschueren]

As follow-up of a discussion started in #986, is the TypeScript definition wrong?

The definition is quite huge, ~1200 lines. The main reason for this is because it prevents TS users to do things like test.serial.serial or test.before.after, etc. But as noted by @thejameskyle, this isn't really representing the runtime structure of AVA.

Also test.serial.serial is not an invalid combination. option-chain is designed so that the latter properties in the chain take precedence.

So although it looks weird and nobody should do it, it's perfectly possible and maybe shouldn't be restricted by the type definition?

These things should/can be prevented by the AVA ESLint rule. This means off course, we should create a AVA TSLint rule as well.

Just wanted to start the discussion over here. Everybody that has useful insights, feedback or whatever, feel free to chime in.

// @ivogabe @sindresorhus @novemberborn @jamestalmage

[jamiebuilds]

Just to emphasize, if you move this to a lint rule where you can express yourself more dynamically you will keep your type definitions small and much more maintainable.

[sindresorhus]

Agreed. In hindsight, it was not worth all the bloat, but it wasn't clear at the time how verbose it would turn out. I don't think anyone would do test.serial.serial anyway and we could indeed have a ESLint rule for it.

we should create a AVA TSLint rule as well.

I think we should wait on typescript-eslint-parser. We don't want to recreate all of our rules in another linter.

[novemberborn]

IMO we should change the runtime so you can't do these silly combinations. Having a stricter TS definition in anticipation of that change seems fine to me.

[jamiebuilds]

@novemberborn I'm having trouble imagining how the logic for that would work in option-chain that wouldn't make the library a bit ridiculous.

Since you define chainable methods like this:

const chainableMethods = {
    foo: {foo: true},
    notFoo: {foo: false},
    bar: {bar: true},
    both: {foo: true, bar: true}
}

I think it's totally reasonable to have a scenario like:

const fn = optionChain({
    broadOption: { foo: true, bar: true, baz: true },
    refinement: { bar: false }
}, ...);

fn.broadOption.refinement(...);

Anything else you'd be looking up which options are set by which method chains and eliminate them based on that which could just be a really confusing result.

The API that exists today makes a ton of sense. I wouldn't change it because you want to enforce a particular code style that people are going to naturally follow regardless. It places a maintenance burden on the type definitions which just makes them impossible to contribute to.

[novemberborn]

@thejameskyle if I'm not mistaken, currently our option-chain usage allows confusing combinations like test.after.beforeEach. I'd like that to raise an error. With typed JS flavors we should be able to exclude those combinations entirely.

[jamiebuilds]

Yeah it does allow that, but you're suggesting a change that impacts more than just that scenario which probably never happens in the real world anyways– and even if it did happen it would work fine.

[rcorrear]

There's also a few things missing, in test.beforeEach/afterEach the param "implementation" (callback) should be as far as I understand a ContextualTest so it provides you with a 'context'; this is preventing me from upgrading to 0.16. Also something that would be nice to have is a before/after[Each] 'always' which is completely missing from the picture and which I guess would have to be added to every namespace that allows it.

Is anyone still working on this? Is there any way I can contribute?

[SamVerschueren]

On mobile now so hard to check, but beforeEach and afterEach shouls return that. What do you get?

And what is missing from the definition? always?

[ivogabe]

@rcorrear context for beforeEach/afterEach was fixed in #1008. I've got a fix for always in #1025.

[ivogabe]

I see that the following questions have been discussed:

1. Should silly combinations be disallowed by the runtime?
2. Should TS block these silly combinations?

The only reason against 1 was that it could break other usages of option-chain. Though I think that it would be easy to add it behind an option or something like that. Then other users can opt-in/opt-out of this behavior.

I think that everyone agrees that if the answer for 1 is yes, then it is also yes for question 2. Otherwise, the TS definitions block constructs that are 'valid' at runtime. Though one could argue that "2" * 3 is also 'valid' at runtime, but still blocked by TypeScript. TypeScript also supports private and protected properties. Even though these are available at runtime, the compiler will block access to them outside of the class declaration.

My ideal solution would be that AVA disallowed these silly combinations. If the validation logic could be exposed somehow to the TS generation script, then the TS definition will be automatically up to date (most times). The generation script already gets the list of all modifiers from the AVA source code, so when a new modifier is added it will automatically be added to the type definitions.

[jamiebuilds]

I think I'll respond to all of that by saying please don't add complexity for complexity's sake when there is no noticeable positive impact to anyone.

Enforcing this means:

Cons:

• Writing generation scripts for types for both Flow and TypeScript
  • Making contribution harder
    • Making it far more likely to fall out of date
• Writing an option to the option-chain library that adds a bunch of complexity
  • You'll have to deal with conflict resolution
  • And you still haven't dealt with broad => specific option groups, which is an api nightmare

Pros:

• People won't be able to do the thing they are already not doing and doesn't cause any issues either way

[novemberborn]

I assume we're using option-chain because it's a nice way to create a chained API. But in actuality we don't want all those combinations to be used, and have written ESLint rules to prevent it. So then why not express the exact API and do away with option-chain?

@avajs/core?