The "The OneSchema importer has already launched" warning doesn't work properly

[csandman]

I've noticed that we've been getting the following warning every time we render the React OneSchemaImporter component:

I tried changing all of the launch params to be statically defined objects outside of the react render flow, but I saw that the warning still occurs. After finally looking at the source to see the logic for how this warning is triggered, I found this:

  useEffect(() => {
    if (importer && importer._hasAttemptedLaunch && isOpen) {
      console.warn(
        "The OneSchema importer has already launched. Updated launch params will not update the current import",
      )
    }
  }, [params, importer, isOpen])

sdk/packages/importer-react/src/OneSchemaImporter.tsx

Lines 155 to 161 in 2bf640f

	useEffect(() => {
	if (importer && importer._hasAttemptedLaunch && isOpen) {
	console.warn(
	"The OneSchema importer has already launched. Updated launch params will not update the current import",
	)
	}
	}, [params, importer, isOpen])

So basically, the warning will trigger any time the importer has already had a launch attempt, and the references to either params or importer or the value of isOpen changes.

The problem with this is that params is redefined every time the function component runs (every render), because its being defined using the ...rest syntax:

export default function OneSchemaImporter({
  isOpen,
  style,
  className,
  inline = true,
  onRequestClose,
  onSuccess,
  onCancel,
  onError,
  onPageLoad,
  onLaunched,
  ...params
}: OneSchemaImporterProps) {

sdk/packages/importer-react/src/OneSchemaImporter.tsx

Lines 80 to 92 in 2bf640f

	export default function OneSchemaImporter({
	isOpen,
	style,
	className,
	inline = true,
	onRequestClose,
	onSuccess,
	onCancel,
	onError,
	onPageLoad,
	onLaunched,
	...params
	}: OneSchemaImporterProps) {

When defined like this, params will never have a stable reference, meaning this will always be triggered if the component is re-rendered, and has already been launched previously, even if the re-render is due to some unrelated change in parent component state.

There are a couple ways you could fix this. One is, you could explicitly define the individual params as in the useEffect dependency array:

useEffect(() => {
  // Only run if actual param values changed
  if (importer && importer._hasAttemptedLaunch && isOpen) {
    console.warn(
      "The OneSchema importer has already launched. Updated launch params will not update the current import",
    )
  }
  // List individual params instead of the whole object
}, [importer, isOpen, params.userJwt, params.templateKey, params.customizationKey, /* etc */])

Or in that same vein, memoize the initialization of the params object based on a similar dependency array, and use that as a dependency for the useEffect.

const memoizedParams = useMemo(() => params, [
  params.userJwt,
  params.templateKey,
  params.customizationKey,
  // ...all param properties
])

Or alternatively, a simpler approach would be to do a deep equality comparison on the params object using a package like react-fast-compare (or whatever alternative deep equals package or utility, could probably just compare JSON stringified strings in this case haha).

import isEqual from "react-fast-compare";

const prevParams = useRef()
useEffect(() => {
  const paramsChanged = !isEqual(prevParams.current, params)
  if (importer && importer._hasAttemptedLaunch && isOpen && paramsChanged) {
    console.warn(
      "The OneSchema importer has already launched. Updated launch params will not update the current import",
    );
  }
  prevParams.current = params
}, [params, importer, isOpen])

This would be the simplest approach because you wouldn't have to list out all of the params manually, and it would be more correct as as it better accounts for some of the object type params changing reference when their values don't actually change, which doesn't really matter in this case due to the importer being defined only once in a state initializer function.

---

Whatever the approach, it would be better than it is right now, because in its current state the warning is pretty misleading.

[afurth89]

I'm seeing the same behavior above, it's very noisy in the logs, would be good to address

[behnam-oneschema]

Thanks for filing this @csandman! Agreed that such a change would improve the developer experience. I think using a third-party deep-compare would make sense, for all the reasons you listed.

We will try to fit this into the next batch of work on the SDK. PRs are also welcome, if you like to make this happen asap.