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

Why is .wrap() not included in the documentation? #2249

Open
jpenna opened this issue Oct 1, 2019 · 15 comments
Open

Why is .wrap() not included in the documentation? #2249

jpenna opened this issue Oct 1, 2019 · 15 comments
Labels
feature request Issues asking for stuff that would be semver-minor help wanted

Comments

@jpenna
Copy link

jpenna commented Oct 1, 2019

Sorry for not following the template, but it is a simple request.

Why is .wrap() not included in the documentation?

As you can see here #919 at least more 6 people found this method useful. Just for completion it should be in the docs. Even if a note is included against the use of it and a suggestion of how to better write the component.

The author suggested to write the PR, I put myself available to do the same.

My use case

My use case is a HOC <Flow> that receives the content component <Step> as a prop. The component <Container>, which renders this HOC, renders the <Step> conditionally based on the current step of a specific flow. I'm using useMemo() to render this component inside the <Container> and that's why I need .wrap() to wrap the <Step> passed as a prop to <Flow>.

I could use a separate function to conditionally render and return <Step>, but this would obscure the code and have the same behavior: this function would be wrapped in a useMemo() and passed as a prop to <Flow>.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

Indeed, see #919 (comment) and #920.

It is intentionally undocumented.

I’m not clear tho on why you need wrap. Can you share some code?

@jpenna
Copy link
Author

jpenna commented Oct 1, 2019

This is a simplified version of the code, but the structure is the basically the same.

I used the same names as the example I gave above, so you can follow that to understand what is going on below if needed.

import React, { useState, useMemo } from 'react';
import Flow from 'components/Flow';
import FlowData from 'components/FlowData';
import { statuses } from 'lib/ducks/meta';

import ChoosePathStep from './steps/ChoosePathStep';
import IssuesStep from './steps/IssuesStep';
import UploadingStep from './steps/UploadingStep';

const Container = ({ uploadStatus }) => {
  const [step, setStep] = useState(() => ChoosePathStep);

  let Step = step;
  if (uploadStatus === statuses.LOADING) {
    Step = UploadingStep;
  } else if (uploadStatus === statuses.FAILURE) {
    Step = IssuesStep;
  }

  const MountedStep = useMemo(() => (
    <Step
      me={me}
      setStep={setStep}
    />
  ), [me, setStep]);

  return (
    <>
      <FlowData
        collapseContentGutter
        rounderBorders
        editable
      />
      <Flow
        blankContent={MountedStep}
      />
    </>
  );
};

export default Container;

@ljharb

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

Can you also share your test code that uses wrap (or doesn't, and doesn't work)?

@jpenna
Copy link
Author

jpenna commented Oct 1, 2019

This is working (simplified/mocked version)

import { shallow } from 'enzyme';
import Flow from 'components/Flow';
import Container from './Container';

import ChoosePathStep from './steps/ChoosePathStep';

describe('Flow', () => {
  it('Renders initial step', () => {
    const wrapper = shallow(<Container />);
    const step = wrapper.wrap(wrapper.find(Flow).prop('blankContent'));
    expect(step.is(ChoosePathStep)).toBe(true);
  });
});

If I change wrapper.wrap() for shallow(), it doesn't work, I don't know why...

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

const Step = () => wrapper.find(Flow).prop('blankContent');
const step = shallow(<Step />);

@jpenna
Copy link
Author

jpenna commented Oct 1, 2019

Nice, it works! I should have tried with a function...

Why would you say this way is preferable instead of using .wrap()?

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

Mainly because wrap isn't intended to be used externally.

A better future enzyme addition would be something like wrapProp('blankContent') that did this for you.

@jpenna
Copy link
Author

jpenna commented Oct 1, 2019

Right.
Yes, that would be good, make it clear how to deal with this scenario.

I took a look into the code, this is quick piece I wrote. I can dive deeper to include this method if it sounds interesting to you.

  wrapProp(prop) {
    if (!prop || typeof prop !== 'string') {
      throw new TypeError('ReactWrapper::wrapProp() expects a string as first argument');
    }
    return this.wrap(this.prop(prop));
  }

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

@jpenna sure, why not :-) can you make a PR to add that (with docs and tests) to both shallow and mount? Please also look to renderProp and invoke for the prop name validations I'd expect.

@jpenna
Copy link
Author

jpenna commented Oct 1, 2019

Cool! I'll work on this this weekend then!

@ljharb ljharb added feature request Issues asking for stuff that would be semver-minor help wanted labels Oct 1, 2019
@jpenna
Copy link
Author

jpenna commented Oct 5, 2019

@ljharb Man, I'm getting a lot of lint errors on the Master branch when I run npm test with React 16. React 15 went fine.

npm run react 16
npm run build
npm test

Just a sample below:

react/destructuring-assignment
  3483:26  warning  Must use destructuring state assignment                                                                                                                                                                                                                                                                      react/destructuring-assignment
  3513:38  warning  Must use destructuring state assignment                                                                                                                                                                                                                                                                      react/destructuring-assignment
  3513:38  warning  Use callback in setState when referencing the previous state    

.../enzyme/packages/enzyme-test-suite/test/shared/methods/setProps.jsx
  260:9   error  componentWillReceiveProps is deprecated since React 16.9.0, use UNSAFE_componentWillReceiveProps instead, see https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops. Use https://github.com/reactjs/react-codemod#rename-unsafe-lifecycles to automatically update your components  react/no-deprecated

@ljharb
Copy link
Member

ljharb commented Oct 5, 2019

Go ahead and ignore those for your PR; any failures on master, I’ll take care of :-)

@jpenna
Copy link
Author

jpenna commented Oct 10, 2019

@ljharb Hey man! Did you see the PR?

@gaplyk
Copy link

gaplyk commented Feb 19, 2021

any updates on wrapProp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues asking for stuff that would be semver-minor help wanted
Projects
None yet
Development

No branches or pull requests

3 participants