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

support testing-library/user-event #10

Closed
smeijer opened this issue May 23, 2020 · 33 comments · Fixed by #209
Closed

support testing-library/user-event #10

smeijer opened this issue May 23, 2020 · 33 comments · Fixed by #209
Assignees
Labels
feature New feature or request

Comments

@smeijer
Copy link
Member

smeijer commented May 23, 2020

I haven't worked out the details, but enabling users to play with user-events would awesome. Perhaps together with adding support for HTML mixed mode?


inspiration: https://twitter.com/kentcdodds/status/1270919381380284416?s=20

@smeijer smeijer added the feature New feature or request label May 23, 2020
@delca85
Copy link
Member

delca85 commented Jun 3, 2020

@smeijer what do you mean with

enabling users to play with user-event

? What would you like to enable users to do?

@smeijer
Copy link
Member Author

smeijer commented Jun 3, 2020

I was thinking that a user should be able to write mixed HTML in the markup editor. Something like:

<label for="checkbox">Check</label>
<input id="checkbox" type="checkbox" />

<button onClick="javascript:document.querySelector('#checkbox').setAttribute('checked', true)">

 Accept
</button>

And that would result in a working demo in the HTML preview pane.

Then just like we can currently use screen.getBy... methods to test locating elements, we could have user-event to let the user click that button, and verify that it worked.

userEvent.click(screen.getByText(/accept/i))

I think we should then also add a 'refresh' button to the preview pane, to undo the changes that have been done by the query.

I'm also not sure how useful this is without having expect, but we can explore adding that later as well. So we can turn the query into something like:

userEvent.click(screen.getByText(/accept/i))
expect(screen.getByLabelText(/check/i)).toHaveAttribute('checked', true)

@delca85
Copy link
Member

delca85 commented Jun 8, 2020

Hey!
I could give it a shot. I don't know if I will be able to accomplish the whole task by myself, if I'll face some blocking problem I will share it!

@delca85
Copy link
Member

delca85 commented Jun 9, 2020

Hi @smeijer!
I am working on this feature and I have a major question: according to you, which is the right way to make the HTML preview panel responds to user-event fired events?
I hope I am explaining myself but I don't see a straight solution to make the code be updated after the event is fired so I don't know if you suppose to catch the event and manually change the element state or if there is something I am missing.
Thank you!

@smeijer
Copy link
Member Author

smeijer commented Jun 9, 2020

Hi @delca85!

What do you have so far? Do you have the mixed html working in code mirror? Does that result in an interactive html preview?

For example, can the user create a button, wich displays an alert on click? I think this is step 1.

Once that's done, (which could be merged individually), we can figure out how to use events.

I think that applying the events isn't the complicated part. The thing we need to figure out is, how to maintain state or restore the html view.

One idea that I have, is to disable real time updates, when we detect a user-event call. And instead show an execute button (which can also be submitted by ctrl enter).

Or just refresh the html preview on every query change. And show a toggle button in the html preview. But I don't know how performant that is. I think this would be the preferred variant.

Does this answer your question?

@delca85
Copy link
Member

delca85 commented Jun 10, 2020

I have added the html mixed mode with CodeMirror and set it as mode in MarkupEditor. I haven't worked yet with nested mode, while it should be used for html mixed as told in CodeMirror manual.
Anyway I am not sure I am deeply understanding the point, because this kind of code:

<label for="checkbox">Check</label>
<input
  id="checkbox"
  type="checkbox"
/>
<button
  onClick="javascript:alert('clicked')"
>
  Accept
</button>

is already working in current playground version. I am sure I am missing something!

I agree with your idea about showing an execute button (and make an execution by ctrl+enter), I am not sure about this but I suppose that as a user I would expect to have a button to make effective the event firing.

@smeijer
Copy link
Member Author

smeijer commented Jun 10, 2020

I haven't worked yet with nested mode

I don't even know what it is 😅, do we need it? What does it do?

I am sure I am missing something!

My bad, that example works. This one doesn't. Should itwork? Can it work?

<label for="checkbox">Check</label>
<input
  id="checkbox"
  type="checkbox"
/>
<button id="btn">
  Accept
</button>

<script>
  const btn = document.getElementById('btn');
  btn.addEventListener('click', () => { alert('click'); });
</script>

@delca85
Copy link
Member

delca85 commented Jun 10, 2020

:D
I suppose that it should work but I need to investigate more! I am going to do it today.

@delca85
Copy link
Member

delca85 commented Jun 10, 2020

The code indentation and styling is working but I need to investigate a little more to make it working in Preview panel.

@smeijer
Copy link
Member Author

smeijer commented Jun 10, 2020

We're setting the html with dangerouslySetInnerHTML, I think react doesn't evaluate the scripts.

Most easy solution that comes to mind, is to extract script elements from the HTML with a regex. Inject the HTML without those script elements. And eval those scripts after the HTML is injected.

@delca85
Copy link
Member

delca85 commented Jun 10, 2020

I am reaching exactly the same idea.

@delca85
Copy link
Member

delca85 commented Jun 10, 2020

I tried an initial implementation with eval and it's working! I am going to clean it a bit. When it will be ready I could push this initial code and start workin on user-event support like you told yesterday.

@smeijer
Copy link
Member Author

smeijer commented Jun 10, 2020

That sounds great 👍 Thanks.

delca85 pushed a commit to delca85/testing-playground that referenced this issue Jun 10, 2020
delca85 pushed a commit to delca85/testing-playground that referenced this issue Jun 10, 2020
@smeijer
Copy link
Member Author

smeijer commented Jun 11, 2020

delca85 pushed a commit to delca85/testing-playground that referenced this issue Jun 11, 2020
@delca85
Copy link
Member

delca85 commented Jun 11, 2020

https://twitter.com/kentcdodds/status/1270919381380284416?s=20

It sounds awesomely useful to apply events!

delca85 pushed a commit to delca85/testing-playground that referenced this issue Jun 12, 2020
@smeijer
Copy link
Member Author

smeijer commented Jun 12, 2020

Yeah, I think we can do something with it. We should figure out how exactly, to make it usefull for more than the development of user-event itself, but I can see the value of debugging events orders.

delca85 pushed a commit to delca85/testing-playground that referenced this issue Jun 12, 2020
@delca85
Copy link
Member

delca85 commented Jun 12, 2020

According to you, one possible way could be dispatching an event for each user-event query found? Should those queries and their args be caught through regex?

@smeijer
Copy link
Member Author

smeijer commented Jun 12, 2020

Sorry, I'm not sure if I understand your question. Can you elaborate?

@delca85
Copy link
Member

delca85 commented Jun 13, 2020

Sorry, you are totally right.

The issue I was speaking about is applied the events to the markup inserted by the user.
Like I told some comments ago, I don't have a precise idea about which is the right way to do that.
Looking at the code I have supposed that one flow could be:

  1. get the user-event method inserted by the user and its args through a regex
  2. create and dispatch an event according to the info retrieved in step 1
  3. execute the code in step 2 on markup inserted by the user
  4. inject the resulting html with dangerouslySetInnerHtml

I don't know if this is a reasonable flow, but I hope I explained myself at least.

@delca85
Copy link
Member

delca85 commented Jun 14, 2020

Hi @smeijer! If it sounds good to you, I am going to keep on with this issue.
Like you know, there is an opened PR that will merge @testing-library/user-event into @testing-library/dom. However I think that I could work anyway on supporting user-event in Testing Playground because where it is located doesn't matter too much.

@smeijer
Copy link
Member Author

smeijer commented Jun 14, 2020

Sure! The migration will (mostly) be backwards compatible. So I don't see why we should wait on user-event being merged.

I expect that once they're merged, we can quite easily migrate/upgrade the references.

@delca85
Copy link
Member

delca85 commented Jun 14, 2020

I expect the same!

Some days ago you told me that according to you applying the event to the markup shouldn't be so hard. Could you maybe share some of your thoughts with me (when you have some free time, obviously!)?

The idea I have about it is the one I exposed here but I would be very happy to know what you think about it.

@smeijer
Copy link
Member Author

smeijer commented Jun 14, 2020

I think that as soon as you add the user-event methods to the sandbox context, it will already work.

When you would than copy/paste a query that includes an user event, you would see the updated preview.

That means, we only need to add a "toggle" to the Preview, to toggle between initial markup, and resulting state.

This should work out of the box, if the preview just reruns the entire query on the markup, on every query change. And gives the user an option to see the effect that his events have on the markup.

But I might be wrong.

@delca85
Copy link
Member

delca85 commented Jun 15, 2020

Hi!
Injecting user-event methods doesn't work exactly out of the box (but it isn't as difficult as I expected :D).
After adding user-event to the evaluator context, I have retrieved the markup produced by the evaluation of the query.
Then I inject the result.markup in Preview.

Right now I am able to execute on this markup:

<label for="checkbox">Check</label>
<input id="checkbox" type="checkbox" />

<button onClick="javascript:document.querySelector('#checkbox').setAttribute('checked', true)">

 Accept
</button>

this query

userEvent.click(screen.getByText(/accept/i))

but I am not managing yet the execution of scripts defined in this html code:

<label for="checkbox">Check</label>
<input
  id="checkbox"
  type="checkbox"
/>
<button id="btn">
  Accept
</button>

<script>
  const btn = document.getElementById('btn');
  btn.addEventListener('click', () => { alert('click'); });
</script>

I suppose I have to work a bit on the scripts evaluation, because I think the problem is related to the moment when the scripts evaluation is performed.
I am writing this to you just to notify that maybe this PR is going to change a bit the part related to the scripts evaluation.

@smeijer
Copy link
Member Author

smeijer commented Jun 15, 2020

I think the trick is in the execution order of the scripts. First evaluate all scripts that have been extracted from the markup. And only after that, apply the queries.

@delca85
Copy link
Member

delca85 commented Jun 16, 2020

Hey @smeijer !
I have achieved the result of executing scripts through user-event methods. To accomplish that I added the scripts evaluation even in runInSandbox in parser.
I haven't a clear idea about how to do that when the html is executed in unsafe mode.

About it I maybe found a small bug: at this line evaluator.exec is called while maybe the right call to do is just to the exec method returned by createEvaluator.

If it sounds good to you I am going to proceed adding the toggle to swap between the original state and the one obtained through the user-event method execution.

@delca85
Copy link
Member

delca85 commented Jun 16, 2020

Now it is working like in this video.
I am going to add a toggle button but I am not so sure where it should be located.
Another thing to be handled is the message shown in ResultSuggestion that in my opinion shouldn't be the same received by a user who hasn't entered any query.

EDIT: I have managed the message shown when a method from user-event is inserted. I am going to put the toggle in the same Result component. I hope I will be able to make the PR this evening!

@smeijer
Copy link
Member Author

smeijer commented Jun 16, 2020

I maybe found a small bug: at this line evaluator.exec is called while maybe the right call to do is just to the exec method returned by createEvaluator.

I'm not sure if that's a bug. Why do you think it is?

Regarding the rest of the comments. I have no idea. Maybe it's easier to commit what you have, and open a pull-request in Draft state? That way we can talk while looking at code. Without the code, it's really hard to get a picture of what you're trying to explain.

I, for example, do not understand why it would be hard to do the same in unsafe mode. sandbox and unsafe share the same createEvaluator. Isn't that where you added user-events to the context?

Let's create a pull-request and talk there further?

@delca85
Copy link
Member

delca85 commented Jun 16, 2020

Yes, sorry for the loss of time.
I have just created a PR.

I'm not sure if that's a bug. Why do you think it is?
I am thinking this because an evaluator.exec method doesn't seem to exist to me, while an exec method is returned by the [createEvaluator](https://github.com/testing-library/testing-playground/blob/494255a42111d2098f470c97ed4628614d714933/src/parser.js#L87) method.

I, for example, do not understand why it would be hard to do the same in unsafe mode. sandbox and unsafe share the same createEvaluator. Isn't that where you added user-events to the context?

The problem in unsafe mode isn't the execution of the user-event methods but the evaluation of the scripts inserted by the user in MarkupEditor that should be evaluated to make the actions dispatched by user-event effective.

I hope this with the code pushed by PR could be more helpful.

@smeijer
Copy link
Member Author

smeijer commented Jun 16, 2020

Ok. That sounds like an acceptable loss for me, as long as we document it in the code.

The unsafe is used for the chrome extension (and soon plugin). We don't have something like "user scripts" there, because all those scripts are already evaluated in the document (chrome tab) before our parser runs.

@delca85
Copy link
Member

delca85 commented Jun 16, 2020

Sorry but I am not sure to understand, the loss you are talking about is the missing evaluation of "use scripts"?

The unsafe is used for the chrome extension (and soon plugin). We don't have something like "user scripts" there, because all those scripts are already evaluated in the document (chrome tab) before our parser runs.

If I got your words properly this is not needed in unsafe mode.

@delca85
Copy link
Member

delca85 commented Jun 16, 2020

I maybe found a small bug: at this line evaluator.exec is called while maybe the right call to do is just to the exec method returned by createEvaluator.

I'm not sure if that's a bug. Why do you think it is?

I am thinking this because an evaluator.exec method doesn't seem to exist to me, while an exec method is returned by the createEvaluator method.

delca85 pushed a commit that referenced this issue Jun 16, 2020
@delca85
Copy link
Member

delca85 commented Jun 17, 2020

Hi!
I am a bit stuck with the execution of user-event queries.
Right now, with the code I pushed in #176 , I am not able to retrieve the code produced after the execution of user-event methods that changes the value prop on an HTML element (e.g. clear, selectOptions...).

According to me the reason is that those methods does not seem to make any transformation on the attribute value (that is like the initial value of the element) of the selected element but they work on the prop value of the element.
This is a small reproduction of the issue.

The interaction requested by @smeijer here are now performed and even the state toggling (before and after the method execution) but right now I am struggling a lot with this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants