RFC: Improve the type system around options in widgets

[ryan-bradford]

Problem

When a user exposed an element using the factory, it created a find function that had one parameter of FindElementOptions. This has an ancestor of unknown and options of UnknownOptions. Within a widget, this makes sense, but external to a widget, these options should be known. In the case of Cypress, the ancestor is a string, and in the case of Angular the ancestor is a DebugElement.

This problem also happens if a user tries to take in options that are passed to a call to click. There was no way for these options to be type safe. They were always unknown.

Solution

It should be possible for these options to be type safe. In a stepdef file, we know we are dealing with Cypress, and in a spec we know we are dealing with Angular.

To accomplish this, the solution was to make the type of the options conditional on the type of entity being returned. Meaning, the type of FindElementOptions is now conditional on the type T passed in. If it is a Chainable, it gives a Cypress options interface; if it is a TestElement it gives an Angular options interface.

This now means two things:

1. From within a widget, it is impossible to make assertions about the type of options you have. Attempting to pass { timeout: x } to a method will throw a type error. This makes sense because widget should not know about the options they accept. If a method needs a changed timeout, these options must be passed from the caller as a WidgetActionOptions<T>. Before because these options were not type safe, passing timeout within a widget would not throw an error.
2. From outside a widget, the type of options is now known. In a Cypress test, the widget knows that it takes in an optional timeout.

In concrete terms, findDataExporter({ options: { timeout: 10000 } }) is valid from a stepdef, but invalid from a unit test. The type of ancestor is also known and type checked.

Example:

Here's a widget that has a few methods that use this new object:

class ExampleWidget<T> extends BaseWidgetObject<T> {
    getHeader = this.factory.css('.header');
    
    private _getInput = this.internalFactory.css('input');
    private _getButton = this.internalFactory.css('button');

    typeInputClickButton(options: WidgetActionOptions<T> {
         this._getInput().type('hello', options);
         this._getButton().click(options);
    }
}

From a stepdef, using this widget would look like:

const widget = findCypressWidget<ExampleWidget<Chainable>>(ExampleWidget, { ancestor: 'test', options: { timeout: 1 } } );
widget.getHeader({ options: { timeout: 1 } });
widget.typeInputClickButton({timeout: 1});

From a unit test, using this widget would look like:

const widget = findCypressWidget<ExampleWidget<Chainable>>(ExampleWidget, { ancestor: someDebugElement });
widget.getHeader({ ancestor: someDebugElement });
widget.typeInputClickButton({timeout: 1}); // <--- does not compile. Timeout is not valid here.

Cons

1. Because Cypress types are global and evil, from within the Cypress widget object element, the options are still unknown. This just makes internal development annoying.
2. The widget object now know about all the possible types of widgets. This new type FindElementOptions needs to know about the different types of widgets so that it can infer which options type to create. This violates some OOP, but is acceptable in my opinion.

Other Options

1. Options are a generic on the class. This makes internal maniplulation of the actions more clear, and it's a simpler pattern. However, all new widgets would need to declare two more generics for O1 and O2 which is not ideal.

Ex:

class DataExporterWidget<T, O, O2> extends BaseWidgetObject<T, O, O2> { ... }

Screenshots (if applicable)

Does this PR introduce a breaking change?

• Yes
• No

FindCypressWidgetOptions is no longer an exported type.

[codecov]

Codecov Report

Merging #334 (7969755) into master (2a03a17) will decrease coverage by 0.02%.
The diff coverage is 87.17%.

❗ Current head 7969755 differs from pull request most recent head 7066195. Consider uploading reports for the commit 7066195 to get more accurate results

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
- Coverage   86.35%   86.33%   -0.03%     
==========================================
  Files         103      103              
  Lines        3343     3344       +1     
  Branches      529      531       +2     
==========================================
  Hits         2887     2887              
+ Misses        296      294       -2     
- Partials      160      163       +3     

Impacted Files	Coverage Δ
...est/widget-object/cypress/cypress-widget-finder.ts	50.00% <25.00%> (-2.64%)	⬇️
...et-object/angular/angular-widget-object-element.ts	80.90% <84.61%> (+1.44%)	⬆️
...nents/src/data-exporter/data-exporter.component.ts	93.96% <100.00%> (-1.69%)	⬇️
...s/components/src/data-exporter/data-exporter.wo.ts	78.26% <100.00%> (+0.98%)	⬆️
...est/widget-object/angular/angular-widget-finder.ts	62.96% <100.00%> (ø)
...et-object/cypress/cypress-widget-object-element.ts	77.50% <100.00%> (ø)
...ents/src/utils/test/widget-object/selector-util.ts	75.00% <100.00%> (ø)
...ents/src/utils/test/widget-object/widget-object.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a03a17...7066195. Read the comment docs.