chore(commands): laid out groundwork for state tracking #210

Andarist · 2021-12-27T10:50:40Z

This builds on top of #201

Andarist · 2021-12-27T10:51:31Z

src/_internalState.ts

@@ -0,0 +1,36 @@
+import {mouseButtonNumbers} from './mouseButtonNumbers'


this is the main reason behind the PR - I'd like to make "state" (like button mask and pressed keys) to be tracked by the library, it's a chore to maintain this on the consumer's side and this would match how Playwright works

Andarist · 2021-12-27T10:52:18Z

src/commands/realClick.ts

+
+  const { clickCount = 1 } = options
+
+  for (let currentClick = 1; currentClick <= clickCount; currentClick++) {


this loop is a fix - gonna prepare a separate PR with this in a moment and when it lands I will rebase this PR on top of that

And there it is: #212

Andarist · 2021-12-27T10:53:51Z

src/commands/realMouseDown.ts

@@ -26,6 +27,32 @@ export interface realMouseDownOptions {
  button?: keyof typeof mouseButtonNumbers;
 }

+export async function rawMouseDown({


I've extracted this so the "composite" realClick command can benefit from the state tracking

Andarist · 2021-12-27T10:56:12Z

src/commands/realPress.ts

-  return {
-    keyCode: keyCode,
-    key: keyDefinition?.key ?? "",
-    text: keyDefinition.key.length === 1 ? keyDefinition.key : undefined,


this line is why this was needed: 5978a6e , after the change to:

- text: keyDefinition.key.length === 1 ? keyDefinition.key : undefined, + text: 'text' in keyDefinition ? keyDefinition.text : undefined,

the special-casing of Enter is not needed anymore, so I've basically reverted the referenced commit

Andarist · 2021-12-27T10:56:40Z

src/commands/realPress.ts

-    if (key.code === "Enter") {
-      await fireCdpCommand("Input.dispatchKeyEvent", {
-        type: "char",
-        unmodifiedText: "\r",
-        text: "\r",
-      });
-    }


see https://github.com/dmtrKovalenko/cypress-real-events/pull/210/files#r775445232

Andarist · 2021-12-27T10:57:16Z

src/commands/realType.ts

@@ -52,8 +43,7 @@ export async function realType(text: string, options: RealTypeOptions = {}) {
    }, [] as string[]);

  for (const char of chars) {
-    assertChar(char);


this isn't needed as getKeyCodeDefinitions throws on unknown characters anyway

Andarist · 2021-12-27T10:58:25Z

src/commands/realType.ts

@@ -52,8 +43,7 @@ export async function realType(text: string, options: RealTypeOptions = {}) {
    }, [] as string[]);

  for (const char of chars) {
-    assertChar(char);
-    await realPress(char, {


I've refactored this to use rawRealPress (that I've introduced in this PR) since I believe that "composite" commands should use "raw" commands over the ones that are exposed publicly as Cypress commands

Andarist · 2021-12-27T10:58:48Z

src/getKeyCodeDefinitions.ts

@@ -0,0 +1,36 @@
+import { keyCodeDefinitions } from "./keyCodeDefinitions";
+
+export type Key = keyof typeof keyCodeDefinitions;


just moved this around and tweaked slightly so both realPress and realType can use this

Andarist · 2021-12-27T11:00:31Z

src/mouseButtonNumbers.ts

@@ -1,8 +1,7 @@
 export const mouseButtonNumbers = {
-    "none": 0,


Since my plan is to track the state - there should be no way to configure "none" mouse buttons for commands like realMouseDown and stuff. Unless this is needed by some kind of mouse input virtualization but I would expect such things to be pretend that some button is pressed so I'm not sure when in practice the none button could be actually used

Andarist · 2021-12-27T11:00:47Z

src/support.ts

+import { realMouseDown } from "./commands/realMouseDown";
+import { realMouseUp } from "./commands/realMouseUp";


I've renamed those files to match other file names

Andarist · 2021-12-27T11:02:58Z

src/support.ts


+Cypress.on('window:before:load', InternalState.clear)


It seems like a reasonable hook to clear the state. I think that this should work (although it's weird):

it('foo', () => { cy.visit('https://...') cy.get('.btn').realMouseDown() }) it('bar', () => { // no visit or anything, piggybacking on the state from the previous test block // performing anything here should assume that the mouse button is still down })

…` and `realMouseUp` (#201)

dmtrKovalenko

I do not understand how this should work on the user side. Can you describe the high level API and the benefits this will bring to the users.

Thanks

Andarist · 2021-12-27T20:28:48Z

My main motivation is that with test scenarios like this:

cy.realKeyDown('ShiftLeft')
cy.realClick()
cy.realKeyUp('ShiftLeft')

I would expect the click to be performed with the shift key being pressed down (e.shiftKey === true). This IMHO helps people to write tests that behave more than the user because this removes the need to pass the information about the shift key "modifier" to the realClick. So it's also less error-prone for users.

dmtrKovalenko · 2022-01-02T12:08:36Z

I don't really like storing a state between commands because:

It's implicit. I mean developer who doesn't know that this library works this way might be confused by the fact that previous command affects the current one
It's uncontrolled. In your implementation there is no clear way to turn off this behavior
It's required breaking change because can affect already existing tests.

For something like this I'd like to see something like this

cy.withRealEventsContext(() => {
  cy.realKeyDown('ShiftLeft')
  cy.realClick()
  cy.realKeyUp('ShiftLeft')
})

In this case if user wants he can do it's own test function that will wrap all the internal commands inside this function. Like

const testReal = (name, fn) => it(name, () => cy.withRealEventsContext(fn))

/// then in test

testReal("it works", () => {
    cy.realKeyDown('ShiftLeft')
    cy.realClick()
    cy.realKeyUp('ShiftLeft')
})

Naming is TBD have no preference there but I'd really like to have control over the mutations that can affect future command

Andarist · 2022-01-02T12:20:08Z

It's implicit. I mean developer who doesn't know that this library works this way might be confused by the fact that previous command affects the current one

I agree that this is implicit but I would argue that this actually helps developers. The very same argument can be said about the opposite - I was actually confused that this sort of stuff is not handled automatically. The consumer of this package is completely not aware of the implementation details, the intricacies of the CDP protocol etc and I think that it makes perfect sense to expect a "lock" (like a key being pressed down) to stay "locked" as long as the lock is not released. This would match the behavior of a lot of existing lock-like APIs out there. This would also match the behavior of both Puppeteer and Playwright.

It's uncontrolled. In your implementation there is no clear way to turn off this behavior

It's a valid concern but also... when would you like to turn this off? At the moment I don't find any compelling scenarios when this would actually be desirable. I expect people to write tests mimicking the user. If a particular sequence of commands cannot be performed by the user then it shouldn't be allowed by the API of this library.

It's required breaking change because can affect already existing tests.

Agreed, but is it a bad thing? Some breaking changes are expected - we can batch this together with some other work. Progress requires breaking changes from time to time - we grow and learn new things about the underlying tools here and it would be a waste not to learn from that and get stuck with the older API for an undefined amount of time, right?

Andarist · 2022-01-24T11:20:43Z

@dmtrKovalenko any more thoughts about this one?

dmtrKovalenko · 2022-02-11T17:06:49Z

Again sorry for long answering but:

I agree that this is implicit but I would argue that this actually helps developers. The very same argument can be said about the opposite - I was actually confused that this sort of stuff is not handled automatically. The consumer of this package is completely not aware of the implementation details, the intricacies of the CDP protocol etc and I think that it makes perfect sense to expect a "lock" (like a key being pressed down) to stay "locked" as long as the lock is not released.

The same works opposite. I personally will expect all the commands to be completely standalone and do not imply on the other commands

It's a valid concern but also... when would you like to turn this off

I would like to avoid this behavior as much as possible and execute one command at a time. Because I know too much about how testing drivers works inside :D. But in fact yes it can confuse somebody when you execute a command and 3 commands after it affects something else.

Make sure that user can do the keydown in a shared command which is out of context

Some breaking changes are expected

I don't think this is a valid reason to make a breaking change for this type of feature

dmtrKovalenko added 2 commits December 2, 2021 18:26

chore(docs): Fix twitter link

a6b3f6c

Merge branch 'main' of https://github.com/dmtrKovalenko/cypress-real-…

7ee4683

…events into develop

Andarist requested a review from dmtrKovalenko December 27, 2021 10:50

Andarist commented Dec 27, 2021

View reviewed changes

Andarist force-pushed the chore/refactor-commands branch from 622c9fa to 827e084 Compare December 27, 2021 11:42

feat(mouse button): Allow to specify button option for `realMouseDown…

b05948e

…` and `realMouseUp` (#201)

Base automatically changed from support-button-option to develop December 27, 2021 18:12

dmtrKovalenko reviewed Dec 27, 2021

View reviewed changes

chore(commands): laid out groundwork for state tracking

48a22b4

Andarist force-pushed the chore/refactor-commands branch from 827e084 to 48a22b4 Compare December 27, 2021 20:23

dmtrKovalenko self-requested a review January 25, 2022 18:56

dmtrKovalenko force-pushed the develop branch from 04b8a35 to 2282480 Compare February 11, 2022 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(commands): laid out groundwork for state tracking #210

chore(commands): laid out groundwork for state tracking #210

Andarist commented Dec 27, 2021 •

edited

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021

Andarist Dec 27, 2021 •

edited

dmtrKovalenko left a comment

Andarist commented Dec 27, 2021

dmtrKovalenko commented Jan 2, 2022 •

edited

Andarist commented Jan 2, 2022

Andarist commented Jan 24, 2022

dmtrKovalenko commented Feb 11, 2022 •

edited

		@@ -0,0 +1,36 @@
		import {mouseButtonNumbers} from './mouseButtonNumbers'


		const { clickCount = 1 } = options

		for (let currentClick = 1; currentClick <= clickCount; currentClick++) {

		@@ -0,0 +1,36 @@
		import { keyCodeDefinitions } from "./keyCodeDefinitions";

		export type Key = keyof typeof keyCodeDefinitions;

		@@ -1,8 +1,7 @@
		export const mouseButtonNumbers = {
		"none": 0,

		import { realMouseDown } from "./commands/realMouseDown";
		import { realMouseUp } from "./commands/realMouseUp";


		Cypress.on('window:before:load', InternalState.clear)

chore(commands): laid out groundwork for state tracking #210

Are you sure you want to change the base?

chore(commands): laid out groundwork for state tracking #210

Conversation

Andarist commented Dec 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist Dec 27, 2021 • edited

Choose a reason for hiding this comment

dmtrKovalenko left a comment

Choose a reason for hiding this comment

Andarist commented Dec 27, 2021

dmtrKovalenko commented Jan 2, 2022 • edited

Andarist commented Jan 2, 2022

Andarist commented Jan 24, 2022

dmtrKovalenko commented Feb 11, 2022 • edited

Andarist commented Dec 27, 2021 •

edited

Andarist Dec 27, 2021 •

edited

dmtrKovalenko commented Jan 2, 2022 •

edited

dmtrKovalenko commented Feb 11, 2022 •

edited