Ability to override default isComponentOfType implementation

[valerybugakov]

Fixes #1148
Let me know if any fixes required.

[javivelasco]

How do you use it? I see there is a variable defined on the global scope of the module but are you overriding through window? The usage should be defined as well. I don't like too much the function to depend on an external variable definition, maybe there is a way to achieve a similar behavior using something more explicit like env variables. This way you transform the source specifying wether you want to use the default checker when you run the transpiler.

[valerybugakov]

Hey, the variable is defined not in global scope but in scope of the module is-component-of-type.js. The usage is pretty simple. In your root React file just import overrideComponentTypeChecker and pass there your checker function. I don't see a way to change the default behaviour without changing an external value. Here's an example from my app with React-hot-loader:

import React from 'react'
import { render } from 'react-dom'
import { AppContainer } from 'react-hot-loader'
import { overrideComponentTypeChecker } from 'react-toolbox'
import store from 'redux/configureStore'
import Root from 'components/Root'

overrideComponentTypeChecker((classType, reactElement) => (
  reactElement && (
    reactElement.type === classType ||
    reactElement.type.name === classType.displayName
  )
))

const throwError = ({ error }) => { throw error }
render(
  <AppContainer errorReporter={throwError}>
    <Root store={store} />
  </AppContainer>,
  document.getElementById('root'),
)

if (module.hot) {
  module.hot.accept('components/Root', () => {
    System.import('components/Root').then(RootModule => {
      const UpdatedRoot = RootModule.default

      render(
        <AppContainer errorReporter={throwError}>
          <UpdatedRoot store={store} />
        </AppContainer>,
        document.getElementById('root'),
      )
    })
  })
}

Let me know what do you think. I didn't get an idea about env approach. How will you provide the checker function then?

[javivelasco]

No your right, I think it makes sense. Now that the example is documented in the same PR we can merge it. Looks good, thanks! :)

[olegstepura]

Such documentation should be in README.
Currently it can be just a link to this PR discussion to allow all users of react-hot-loader v3 to use react-toolbox without issues.

[olegstepura]

I suggest this change:

Usage with react-hot-loader v3

Since react-hot-loader v3 babel patch leads to issues with some components (see #1152, #1155) to allow hot reloading in dev mode you need to define your own type checker in your main entry point (see #1164 for details):

import "react-hot-loader/patch"
import React from 'react'
import { overrideComponentTypeChecker } from 'react-toolbox'

overrideComponentTypeChecker((classType, reactElement) => (
  reactElement && (
    reactElement.type === classType ||
    reactElement.type.name === classType.displayName
  )
))

[javivelasco]

Looks good! PR?

[olegstepura]

I was thinking about this addition. And here are my thoughts.

1. I don't like the idea to force usage of overrideComponentTypeChecker in main entry point.
   • Maybe we can just add this code ( || reactElement.type.name === classType.displayName) to the default type checker. Not sure if this can lead to issues.
   • Or maybe we can use a switch to detect react hot loader and use custom code. E.g.

   export function defaultChecker (classType, reactElement) {
     if (typeof global.__REACT_HOT_LOADER__ !== 'undefined') { // or if (process.env.NODE_ENV !== 'production')
       return reactElement && (
         reactElement.type === classType ||
         reactElement.type.name === classType.displayName
       )
     } else {
       return reactElement && reactElement.type === classType
     }
   }
   
       

2. If we cannot avoid focing users to add this extra code in main file (why?), maybe we can provide a ready method for not to write this copy-paste. Like overrideComponentTypeChecker(hotLoaderTypeChecker) or just setComponentTypeCheckerForHotLoader()

[javivelasco]

It shouldn't create any issues. Do you want to PR this fix? Makes total sense

[valerybugakov]

It would be great to still have an ability to change defaultTypeChecker by hand or to switch it into displayName mode manually. I think there may be other cases except of React-hot-loader compat.

[olegstepura]

@javivelasco which version do you want as a PR?
@valerybugakov Ability to overwrite will be preserved for sure ;)

[olegstepura]

Ok, my vision is as such:

import global from 'global';

let customChecker;
const hotLoaderUsed = typeof global.__REACT_HOT_LOADER__ !== 'undefined';

export function overrideComponentTypeChecker (providedChecker) {
  customChecker = providedChecker;
}

export function defaultChecker (classType, reactElement) {
  return reactElement && reactElement.type === classType;
}

export function defaultCheckerWithDisplayNameCheck(classType, reactElement) {
  return reactElement && (
    reactElement.type === classType
    || reactElement.type.name === classType.displayName
  );
}

export default function isComponentOfType (classType, reactElement) {
  if (customChecker) {
    return customChecker(classType, reactElement);
  } else if (hotLoaderUsed) {
   return defaultCheckerWithDisplayNameCheck(classType, reactElement);
  } else {
    return defaultChecker(classType, reactElement);
  }
}

please comment if you accept this (with an extra dep 'global') and if you accept code without semicolons . I will provide a PR afterwards.

[javivelasco]

To me it looks ok and functional. But it needs to be adapted to the code style defined by eslint which basically it's airbnbs (with semicolons)

[olegstepura]

ok, I'll do that

[javivelasco]

Actually I'm not sure why shouldn't we just make the check in the same function.

[olegstepura]

please decide ;)
Not sure, but issues could raise out of minification (if comparing type.name would lead to wrong positives)
Related: hot-loader issue and react-devtools issue

[javivelasco]

I'm not sure about this anyway. I was trying the solution in a project with react-hot-loader and it didn't work nicely. Maybe we can just keep the ability to override the function so anybody can write its own.

[olegstepura]

Maybe point 2 from #1164 (comment) is what we need? E.g. overrideComponentTypeChecker(hotLoaderTypeChecker)

[javivelasco]

If it helps people with hot loader it's ok on my side :)