Remove props from placeholder div

[bfin]

Hi!

I'm passing props to my LoadableComponent elements, but those props are also passed to the placeholder div elements so I'm getting some annoying warnings:

Warning: React does not recognize the `myCustomProp` prop on a DOM element.
If you intentionally want it to appear in the DOM as a custom attribute, 
spell it as lowercase `myCustomProp` instead. If you accidentally passed it
from a parent component, remove it from the DOM element.

Should be easy enough to remove these warnings by deleting lines 80 and 96 in createLoadableVisibilityComponent.js, but the better way would probably be to extract the className prop and continue to apply it to the placeholder div (i.e., {...props} -> className={props.className}) in case anyone is relying on that functionality (also in your tests, I believe).

Larger code block for context:

    if (LoadingComponent || props.fallback) {
      return (
        <div
          style={{
            display: "inline-block",
            minHeight: "1px",
            minWidth: "1px"
          }}
          className={props.className}  #CHANGE: previously {...props}
          ref={visibilityElementRef}
        >
          {LoadingComponent
            ? React.createElement(LoadingComponent, {
                isLoading: true,
                ...props
              })
            : props.fallback}
        </div>
      );
    }

    return (
      <div
        style={{ display: "inline-block", minHeight: "1px", minWidth: "1px" }}
        className={props.className}  #CHANGE: previously {...props}
        ref={visibilityElementRef}
      />
    );
  }

Is there another reason for applying the props to the placeholder div elements? Happy to create a PR if you're in agreement.

[tazsingh]

Hey!

Thanks for the description of the problem 👌🏽

In my opinion, if the fix in this scenario doesn’t break any existing tests, I’m fine with it. I don’t believe it will.

However an additional test should be added to describe this behaviour for future reference.

Would you be able to incorporate that into your PR? Happy to help via the review process for what you send up.

Sent from my iPhone

On Sep 25, 2019, at 7:33 PM, Brian Findlay notifications@github.com wrote:

 Hi!

I'm passing props to my LoadableComponent elements, but those props are also passed to the placeholder div elements so I'm getting some annoying warnings:

Warning: React does not recognize the myCustomProp prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase myCustomProp instead. If you accidentally passed it from a parent component, remove it from the DOM element. Should be easy enough to remove these warnings by deleting lines 80 and 96 in createLoadableVisibilityComponent.js, but the better way would probably be to extract the className prop and continue to apply it to the placeholder div (i.e., {...props} -> className={props.className}) in case anyone is relying on that functionality (also in your tests, I believe).

Larger code block for context:

if (LoadingComponent || props.fallback) {
  return (
    <div
      style={{
        display: "inline-block",
        minHeight: "1px",
        minWidth: "1px"
      }}
      className={props.className}  #CHANGE: previously {...props}
      ref={visibilityElementRef}
    >
      {LoadingComponent
        ? React.createElement(LoadingComponent, {
            isLoading: true,
            ...props
          })
        : props.fallback}
    </div>
  );
}

return (
  <div
    style={{ display: "inline-block", minHeight: "1px", minWidth: "1px" }}
    className={props.className}  #CHANGE: previously {...props}
    ref={visibilityElementRef}
  />
);

} Is there another reason for applying the props to the placeholder div elements? Happy to create a PR if you're in agreement.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[bfin]

@tazsingh Looking at this a little more, it's just a tad more complicated so I'd like your guidance before moving forward with the PR.

queryByTestId relies on the temporary div element receiving the data-testid data attribute, so we can't just pull out the className prop and apply that to both the div and the loadable component (we'd also have to pull out that data attribute). This was a common challenge with HOCs and the usual solution was to use wrapperProps to explicitly identify anything we only want the wrapper component (in this case the outer div) to receive.

Example usage:

const LoadableComponent = loadableVisibility(() => import("./my-component"), {
  fallback: <Loading />
});

export default function App() {
  return (
    <LoadableComponent
      wrapperProps={{
        htmlAttribute: 'onlyTheDivWillSeeThis',
        className: 'onlyTheDivWillSeeThis',
      }}
      className='onlyTheLoadableComponentWillSeeThis'
      myCustomProp='onlyTheLoadableComponentWillSeeThis'
    />;
}

I think wrapperProps would be the appropriate way to pass a className to the temporary div element, but we could apply it to both by joining the 2 potential values (i.e. className={[props.className, props.wrapperProps.className].join(' ')}).

Example usage:

const LoadableComponent = loadableVisibility(() => import("./my-component"), {
  fallback: <Loading />
});

export default function App() {
  return (
    <LoadableComponent
      wrapperProps={{
        htmlAttribute: 'onlyTheDivWillSeeThis',
        className: 'onlyTheDivWillSeeThis',
      }}
      className='bothWillSeeThis'
      myCustomProp='onlyTheLoadableComponentWillSeeThis'
    />;
}

What are your thoughts on this?

[tazsingh]

Hmmm interesting. Thank you for laying out the problem space as such, I'm starting to see the issue.

I don't have a clear thought right now but let me sit on this until next week if it's not urgent? I need a bit of a clearer head to fully grok a recommended path forward 🙏

[BenBish]

@tazsingh we have just encountered this problem too. Have you given any thought to a solution?

[tazsingh]

@BenBish Thanks for poking me on this!

I think I'm leaning towards taking a render props approach, as I believe it to be the cleanest way to resolve particular props being passed to particular elements.

For example:

const MyComponent = () => {
  return <LoadableVisibilityComponent {...propsForTheWrappingDiv}>{
    () => {
        // This will only be executed once the component is visible
      return <MyLoadableComponent {...propsOnlyForThisElement} />
    }
  }</LoadableVisibilityComponent>
}

However, the big challenge with that approach is that this library is meant to simply wrap react-visibility and @loadable/component with an additive API (if any) on top of theirs. The above render props API is certainly a divergence from that.

This may be fine as the API would be even more flexible as you can technically put whichever lib inside the render prop (such as a React.lazy()), but it would certainly be a breaking change and a big shift for this library. I'm happy to move in that direction if we all feel that it's better? Old versions of this lib wouldn't be going anywhere.

The alternative is what @bfin has kindly detailed above - to provide another prop to just handle the wrapper props and combine both sets of props where it makes sense. I believe that to be the least invasive but comes at the cost of a larger surface area and potential ambiguity around combining props (e.g. Do we always list the wrapper className before the underlying className if both are provided? Would that cause specificity issues? Maybe it's a non-issue as someone could create 2 distinct classNames, but then we'd need to document this behaviour?).

What do you think?

[AmyScript]

Any fix for this? Running into this same issue...

[tazsingh]

@AmyScript I believe the proper solution to be what I detailed in the above comment where I've also listed the considerations for such an approach. What do you think?

[AmyScript]

I like the approach @tazsingh I'm not sure what kind of props is needed to pass into the Wrapper usually? I don't have a use case for passing props into the Wrapper.

Perhaps we can start with just a PR to pass props to the original component?

[tazsingh]

@AmyScript It looks like @bfin attempted that PR to pass props to the underlying component and ran into the issue described in his comment above - https://github.com/stratiformltd/react-loadable-visibility/issues/29#issuecomment-535492703

If you can elegantly resolve that issue in a PR, I'd be more than happy to review it!

However, I am doubtful that it can be solved elegantly without essentially doing what I recommended in my comment and by introducing those tradeoffs.... Not to say you shouldn't try!