Skip to content

Commit

Permalink
fix: make type comparison stronger
Browse files Browse the repository at this point in the history
  • Loading branch information
theKashey committed Jul 4, 2019
1 parent 045a27f commit 1b9f2da
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 22 deletions.
30 changes: 30 additions & 0 deletions src/internal/reactUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,34 @@ export const isForwardType = ({ type }) =>
type && typeof type === 'object' && '$$typeof' in type && type.$$typeof === ForwardType && ForwardType;
export const isContextType = type => isContextConsumer(type) || isContextProvider(type);

export const getElementType = type => {
const element = { type };

if (isContextConsumer(element)) {
return 'Consumer';
}
if (isContextProvider(element)) {
return 'Provider';
}
if (isLazyType(element)) {
return 'Lazy';
}
if (isMemoType(element)) {
return 'Memo';
}
if (isForwardType(element)) {
return 'Forward';
}

if (isReactClass(type)) {
return 'Class';
}

if (typeof element === 'function') {
return 'FC';
}

return 'unknown';
};

export const getContextProvider = type => type && type._context;
55 changes: 33 additions & 22 deletions src/reconciler/componentComparator.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getIdByType, getProxyByType, getSignature, isColdType, updateProxyById } from './proxies';
import { hotComparisonOpen } from '../global/generation';
import {
getElementType,
isContextType,
isForwardType,
isLazyType,
Expand All @@ -20,31 +21,36 @@ const getInnerComponentType = component => {
};

function haveEqualSignatures(prevType, nextType) {
const prevSignature = getSignature(prevType);
const nextSignature = getSignature(nextType);
try {
const prevSignature = getSignature(prevType);
const nextSignature = getSignature(nextType);

if (prevSignature === undefined && nextSignature === undefined) {
return true;
}
if (prevSignature === undefined || nextSignature === undefined) {
return false;
}
if (prevSignature.key !== nextSignature.key) {
return false;
}

// TODO: we might need to calculate previous signature earlier in practice,
// such as during the first time a component is resolved. We'll revisit this.
const prevCustomHooks = prevSignature.getCustomHooks();
const nextCustomHooks = nextSignature.getCustomHooks();
if (prevCustomHooks.length !== nextCustomHooks.length) {
return false;
}
if (prevSignature === undefined && nextSignature === undefined) {
return true;
}
if (prevSignature === undefined || nextSignature === undefined) {
return false;
}
if (prevSignature.key !== nextSignature.key) {
return false;
}

for (let i = 0; i < nextCustomHooks.length; i++) {
if (!haveEqualSignatures(prevCustomHooks[i], nextCustomHooks[i])) {
// TODO: we might need to calculate previous signature earlier in practice,
// such as during the first time a component is resolved. We'll revisit this.
const prevCustomHooks = prevSignature.getCustomHooks();
const nextCustomHooks = nextSignature.getCustomHooks();
if (prevCustomHooks.length !== nextCustomHooks.length) {
return false;
}

for (let i = 0; i < nextCustomHooks.length; i++) {
if (!haveEqualSignatures(prevCustomHooks[i], nextCustomHooks[i])) {
return false;
}
}
} catch (e) {
logger.error('React-Hot-Loader: error occurred while comparing hook signature', e);
return false;
}

return true;
Expand Down Expand Up @@ -99,7 +105,12 @@ const areDeepSwappable = (oldType, newType) => {
const compareComponents = (oldType, newType, setNewType, baseType) => {
let defaultResult = oldType === newType;

if ((oldType && !newType) || (!oldType && newType) || typeof oldType !== typeof newType) {
if (
(oldType && !newType) ||
(!oldType && newType) ||
typeof oldType !== typeof newType ||
getElementType(oldType) !== getElementType(newType)
) {
return defaultResult;
}

Expand Down

1 comment on commit 1b9f2da

@BerndWessels
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theKashey Hi, I believe that this change breaks the very popular https://material-ui.com

Hot reloading with anything ripple related even like just a simple Button in it now results in:

index.js:1 React-Hot-Loader: error occurred while comparing hook signature ReferenceError: useRippleHandler is not defined
    at Object.getCustomHooks (ButtonBase.js:65)
    at haveEqualSignatures (react-hot-loader.development.js:2560)
    at areSignaturesCompatible (react-hot-loader.development.js:2585)
    at compareRegistered (react-hot-loader.development.js:2593)
    at compareComponents (react-hot-loader.development.js:2634)
    at hotComponentCompare (react-hot-loader.development.js:2725)
    at reconcileSingleElement (react-dom.development.js:15478)
    at reconcileChildFibers (react-dom.development.js:15566)
    at reconcileChildren (react-dom.development.js:18076)
    at updateForwardRef (react-dom.development.js:18142)
    at beginWork$1 (react-dom.development.js:20185)
    at beginWork$$1 (react-dom.development.js:25713)
    at performUnitOfWork (react-dom.development.js:24661)
    at workLoopSync (react-dom.development.js:24637)
    at performSyncWorkOnRoot (react-dom.development.js:24236)
    at react-dom.development.js:12181
    at unstable_runWithPriority (scheduler.development.js:818)
    at runWithPriority$2 (react-dom.development.js:12131)
    at flushSyncCallbackQueueImpl (react-dom.development.js:12176)
    at flushSyncCallbackQueue (react-dom.development.js:12164)
    at Object.batchedUpdates$1 [as unstable_batchedUpdates] (react-dom.development.js:24358)
    at react-hot-loader.development.js:3045
    at react-hot-loader.development.js:3004 
    in WithStyles(ForwardRef(ButtonBase)) (created by Button)
    in Button (created by WithStyles(ForwardRef(Button)))
    in WithStyles(ForwardRef(Button)) (at root.js:55)
    in Root (created by HotExportedRoot)
    in HotExportedRoot (at src/​index.js:13)
console.<computed> @ index.js:1
r @ backend.js:6
error @ react-hot-loader.development.js:294
haveEqualSignatures @ react-hot-loader.development.js:2572
areSignaturesCompatible @ react-hot-loader.development.js:2585
compareRegistered @ react-hot-loader.development.js:2593
compareComponents @ react-hot-loader.development.js:2634
hotComponentCompare @ react-hot-loader.development.js:2725
reconcileSingleElement @ react-dom.development.js:15478
reconcileChildFibers @ react-dom.development.js:15566
reconcileChildren @ react-dom.development.js:18076
updateForwardRef @ react-dom.development.js:18142
beginWork$1 @ react-dom.development.js:20185
beginWork$$1 @ react-dom.development.js:25713
performUnitOfWork @ react-dom.development.js:24661
workLoopSync @ react-dom.development.js:24637
performSyncWorkOnRoot @ react-dom.development.js:24236
(anonymous) @ react-dom.development.js:12181
unstable_runWithPriority @ scheduler.development.js:818
runWithPriority$2 @ react-dom.development.js:12131
flushSyncCallbackQueueImpl @ react-dom.development.js:12176
flushSyncCallbackQueue @ react-dom.development.js:12164
batchedUpdates$1 @ react-dom.development.js:24358
(anonymous) @ react-hot-loader.development.js:3045
(anonymous) @ react-hot-loader.development.js:3004
Promise.then (async)
add @ react-hot-loader.development.js:3003
deepUpdate @ react-hot-loader.development.js:3061
Promise.then (async)
updateInstances @ react-hot-loader.development.js:3099
(anonymous) @ react-hot-loader.development.js:3113
hotSetStatus @ bootstrap:267
hotApply @ bootstrap:652
(anonymous) @ bootstrap:363
Promise.then (async)
hotUpdateDownloaded @ bootstrap:362
hotAddUpdateChunk @ bootstrap:338
webpackHotUpdateCallback @ bootstrap:57
(anonymous) @ main.def61cd85fc84d73050d.hot-update.js:1

code to investigate would be @material-ui/core/es/ButtonBase.js

This is with all the latest dependencies

{
  "browserslist": {
    "development": [
      "last 1 chrome version",
      "last 1 explorer version",
      "last 1 firefox version",
      "last 1 safari version"
    ],
    "production": [
      "last 1 chrome version",
      "last 1 explorer version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  },
  "dependencies": {
    "@material-ui/core": "^4.8.3",
    "@material-ui/icons": "^4.5.1",
    "@testing-library/jest-dom": "^5.0.0",
    "@testing-library/react": "^9.4.0",
    "@testing-library/user-event": "^8.0.3",
    "ramda": "^0.26.1",
    "react": "^16.12.0",
    "react-dom": "^16.12.0",
    "react-intl": "^3.11.0",
    "react-router-dom": "^5.1.2",
    "react-scripts": "3.3.0"
  },
  "eslintConfig": {
    "extends": "react-app"
  },
  "name": "material",
  "private": true,
  "scripts": {
    "build": "craco build",
    "eject": "craco eject",
    "start": "craco start",
    "test": "craco test"
  },
  "version": "0.1.0",
  "devDependencies": {
    "@craco/craco": "^5.6.3",
    "@hot-loader/react-dom": "^16.11.0",
    "handlebars": "^4.7.2",
    "react-hot-loader": "^4.12.18"
  }
}

Please sign in to comment.