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

Rework type #581

Merged
merged 43 commits into from
Mar 16, 2021
Merged

Rework type #581

merged 43 commits into from
Mar 16, 2021

Conversation

ph-fritsche
Copy link
Member

@ph-fritsche ph-fritsche commented Mar 9, 2021

What:

This PR will include a complete rewrite of userEvent.type to tackle the issues tracked in #521

There are a lot of changes and some of the fixes along the way might break code that considered these bugs features. So a major version update should happen.

With the changes userEvent.type will actually return no Promise if there is no delay.

The newly introduces userEvent.keyboard skips all convenience of userEvent.type and just applies the key events on the active element.
It allows to apply events and continue with a keyboard state.

it('continue typing with state', () => {
const {element, getEventSnapshot, clearEventCalls} = setup('<input/>')
;(element as HTMLInputElement).focus()
clearEventCalls()
const state = userEvent.keyboard('[ShiftRight>]')
expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value=""]
input[value=""] - keydown: Shift (16) {shift}
`)
clearEventCalls()
userEvent.keyboard('F[/ShiftRight]', {keyboardState: state})
expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value="F"]
input[value=""] - keydown: F (70) {shift}
input[value=""] - keypress: F (70) {shift}
input[value="F"] - input
"{CURSOR}" -> "F{CURSOR}"
input[value="F"] - keyup: F (70) {shift}
input[value="F"] - keyup: Shift (16)
`)
})

Why:

userEvent.type currently is a patchwork of specialized implementations for edge cases.
We've got a bunch of open issues that nobody wants to tackle because it is unclear where exactly a change should be implemented and if there are more things to consider.
The code lacks structure and documentation.

The conversion to Typescript highlighted a lot of hidden debt inside the code regarding assumptions about return values passed around without further checks.

How:

  • First Step: Split utils and type into multiple files and convert to Typescript to improve confidence in upcoming changes
  • Refactor the typeImplementation to provide a shared internal API to extend for special characters, meta characters, etc
  • Convert handler implementations to new plugins
  • Add tests for uncovered lines
  • Introduce new API
  • Add documentation

Checklist:

  • Documentation
  • Tests
  • Typings
  • Ready to be merged

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #581 (f6c533e) into master (e83d949) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #581    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           13        47    +34     
  Lines          728       855   +127     
  Branches       232       323    +91     
==========================================
+ Hits           728       855   +127     
Impacted Files Coverage Δ
src/index.js 100.00% <ø> (ø)
src/keyboard/getEventProps.ts 100.00% <100.00%> (ø)
src/keyboard/getNextKeyDef.ts 100.00% <100.00%> (ø)
src/keyboard/index.ts 100.00% <100.00%> (ø)
src/keyboard/keyMap.ts 100.00% <100.00%> (ø)
src/keyboard/keyboardImplementation.ts 100.00% <100.00%> (ø)
src/keyboard/plugins/arrow.ts 100.00% <100.00%> (ø)
src/keyboard/plugins/character.ts 100.00% <100.00%> (ø)
src/keyboard/plugins/control.ts 100.00% <100.00%> (ø)
...eyboard/plugins/control/calculateNewDeleteValue.ts 100.00% <100.00%> (ø)
... and 65 more

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 e83d949...f6c533e. Read the comment docs.

@ph-fritsche ph-fritsche mentioned this pull request Mar 11, 2021
8 tasks
@ph-fritsche
Copy link
Member Author

@adamayres The rewrite is complete. All previous tests (apart from a) a bug and b) an unlogic edge case) pass.
Your bug should be fixed by the new implementation too.

This was referenced Mar 18, 2021
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.

3 participants