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

Invoke() api #1856

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@koba04
Copy link
Contributor

koba04 commented Oct 8, 2018

This PR is to pick up from #945
If @milesj is still working on #945, feel free to close this!

@milesj

This comment has been minimized.

Copy link
Contributor

milesj commented Oct 9, 2018

I haven't had time to work on my PR, so thanks for this!

const wrapper = shallow(<Foo />);
wrapper.find('a').invoke('click').then(() => {

This comment has been minimized.

@woutervanvliet

woutervanvliet Oct 19, 2018

In the unit tests, invoke is used as below

wrapper.find('button').invoke('onClick')

Do they both work?

});
it('can return the handlers return value', () => {
const spy = sinon.stub().returns(123);
class Foo extends React.Component {

This comment has been minimized.

@woutervanvliet

woutervanvliet Oct 19, 2018

You can write shorter test by using a pure function component instead.

This comment has been minimized.

@ljharb

ljharb Oct 20, 2018

Member

Using an SFC means the tests can't run in React 0.13; so most of our tests should be using class-based components.

render() {
return (
<div>
<a onClick={spy}>foo</a>

This comment has been minimized.

@woutervanvliet

woutervanvliet Oct 19, 2018

Maybe change this to a button. There's a real risk that somebody will look at this code and use it as an example - and then are stuck with bad HTML semantics and potentially an app that doesn't work (don't think tags are clickable without href)

return this.single('invoke', () => {
const handler = this.prop(propName);
if (typeof handler !== 'function') {
throw new TypeError('ReactWrapper::invoke() expects a function name as its first argument');

This comment has been minimized.

@woutervanvliet

woutervanvliet Oct 19, 2018

Doesn't it expect a prop name as first argument?

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Oct 19, 2018

@woutervanvliet Thank you for your reviews! I've fixed them.

@ljharb
Copy link
Member

ljharb left a comment

This will need a full rebase - not just a merge - on latest master.

#### Arguments

1. `propName` (`String`): The function prop to be invoked
2. `...args` (`Any` [optional]): Arguments that will be passed to the prop function

This comment has been minimized.

@ljharb

ljharb Nov 12, 2018

Member

Using a spread here prevents ever being able to pass options.

The pattern we went with in renderProp was to return a new function, which could then be invoked with a variable number of arguments. What do you think about doing that here?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 7, 2019

@koba04 ping, still interested in this?

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Mar 7, 2019

@ljharb OK, I'll work on it again!

@koba04 koba04 force-pushed the koba04:invoke-api branch from 620722c to 02c2b1b Mar 7, 2019

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Mar 7, 2019

I've recreated the branch from master.
Next, I'll adress #1856 (comment)

@koba04 koba04 force-pushed the koba04:invoke-api branch from 02c2b1b to 1eecdaf Mar 9, 2019

@koba04 koba04 force-pushed the koba04:invoke-api branch from 1eecdaf to 5e28ce4 Mar 9, 2019

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Mar 10, 2019

@ljharb I’ve addressed you comment and rebased this!

Show resolved Hide resolved docs/api/ReactWrapper/invoke.md Outdated
Show resolved Hide resolved docs/api/ShallowWrapper/invoke.md Outdated
Show resolved Hide resolved docs/api/mount.md
Show resolved Hide resolved docs/api/shallow.md
Show resolved Hide resolved packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated
Show resolved Hide resolved packages/enzyme/src/ReactWrapper.js Outdated
}
return (...args) => {
const response = handler.apply(this, args);
this[ROOT].update();

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

why is this needed?

This comment has been minimized.

@koba04

koba04 Mar 18, 2019

Author Contributor

@ljharb
The purpose is to reflect the updates into the ShallowWrapper instance if the handler calls setState.

It's the same as what simulate() is doing.

https://github.com/airbnb/enzyme/blob/master/packages/enzyme/src/ShallowWrapper.js#L999

But I've covered the most case by this PR so this might be unnecessary.

#1499

Show resolved Hide resolved packages/enzyme/src/ShallowWrapper.js Outdated
Show resolved Hide resolved packages/enzyme/src/ShallowWrapper.js Outdated
}
return (...args) => {
const response = handler.apply(this, args);
this[ROOT].update();

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

ditto

ljharb and others added some commits Mar 18, 2019

Apply suggestions from code review
Co-Authored-By: koba04 <koba0004@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.