yesmeck / react-with-hooks

[OUTDATED]Ponyfill for the React Hooks API (Support RN)
https://codesandbox.io/s/olx6zp44n6
MIT License
152 stars 11 forks source link

Add polyfill #5

Closed DuncanMacWeb closed 5 years ago

DuncanMacWeb commented 5 years ago

This adds a polyfill so that react-with-hooks can be used in exactly the same way as native Hooks in React 16.8 — except that you import from react-with-hooks/polyfill instead of from react — as described in the readme. It works by storing hookified functional components in a Map.

DuncanMacWeb commented 5 years ago

@yesmeck A couple of points to cover, but I first wanted to check if this is something you would be interested in merging:

Feel free to push changes to this polyfill branch to amend this PR, if you like.

codecov[bot] commented 5 years ago

Codecov Report

Merging #5 into master will increase coverage by 85.31%. The diff coverage is 92.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #5       +/-   ##
===========================================
+ Coverage   12.24%   97.56%   +85.31%     
===========================================
  Files           7        9        +2     
  Lines         449      492       +43     
  Branches       83       92        +9     
===========================================
+ Hits           55      480      +425     
+ Misses        324       12      -312     
+ Partials       70        0       -70
Impacted Files Coverage Δ
src/scheduleCallback.js 0% <0%> (ø)
src/withHooks.js 97.93% <100%> (+93.17%) :arrow_up:
src/polyfill.js 100% <100%> (ø)
src/ReactHooks.js 100% <0%> (+100%) :arrow_up:
src/objectIs.js 100% <0%> (+100%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f136a01...01448a9. Read the comment docs.

DuncanMacWeb commented 5 years ago

Brilliant work @yesmeck, thank you!! Just a heads up: I had to remove the install/publish hooks in package.json to make this install correctly (based on e92a90c — the prepack step errored on npm install because npm could not find the babel command, and scripts/alertFlag.sh errored because I actually didn’t have node_modules/react-reconciler/cjs/react-reconciler.development.js in my project directory, but after that everything is working fine! 👍

I would suggest moving the build step to another npm hook such as prepublishOnly.

DuncanMacWeb commented 5 years ago

@yesmeck I also noticed today that your master branch version now wraps components that are outside the strict subset “functional components that use Hooks” which the typecheck and regex previously enforced. It would be good to reapply this subset somehow, so that the overhead of withHooks is only applied where it’s really needed.

yesmeck commented 5 years ago

Regex won't work for custom hooks.

Typecheck https://github.com/yesmeck/react-with-hooks/blob/master/src/polyfill.js#L39

DuncanMacWeb commented 5 years ago

Why won’t regex work? :)

yesmeck commented 5 years ago

https://github.com/yesmeck/react-with-hooks/pull/5/commits/90a3dd57354f9719d69c02f87fb951047c2896a8#diff-1c3729c9686c9bb355842fc03669676aR13

Because the regex in this commit only checked the built-in hooks.

yesmeck commented 5 years ago

BTW, what kind of project do you use this library? Why don't you use the official version?

I found some behaviors of hooks cannot be achieved on the userland.

DuncanMacWeb commented 5 years ago

Ah yes, good point. So how about adapting the regex in eslint-plugin-react-hooks; something like /\buse[A-Z0-9].*\b/?

yesmeck commented 5 years ago

Looks good for me.

yesmeck commented 5 years ago

No \b since after uglify, whitespaces will be removed.

DuncanMacWeb commented 5 years ago

I’m using it at work on a project where we have agreed to use a uniform version of React among all teams across our site, so that the user doesn’t have to download multiple React versions, and there will be a delay before we can agree to upgrade to 16.8. But I’m also interested in potentially using it in React Native, or in NativeScript using preact-to-nativescript.

DuncanMacWeb commented 5 years ago

Actually I think \b does not depend on whitespace.

yesmeck commented 5 years ago

So, why do we need \b?

DuncanMacWeb commented 5 years ago

Perhaps we don’t need it at the end, but at the start it’s to identify the start of hook function names. There would be a word boundary after ; for example. I think we could perhaps replace the later \b with \(.

yesmeck commented 5 years ago

useState will be compiled to t.useState by webpack. The ending \( looks good.

DuncanMacWeb commented 5 years ago
> 't.ueeState()'.match(/\buse[A-Z0-9]\w+\(/)
null
> 't.useState()'.match(/\buse[A-Z0-9]\w+\(/)
[ 'useState(',
  index: 2,
  input: 't.useState()',
  groups: undefined ]
>
yesmeck commented 5 years ago

\( also won't work after babel compiling.

function PassiveEffect(props) {
  useEffect(() => {
    logger.yield(`Passive effect`);
  }, []);
  return <Text text="Passive" />;
}

After compiling:

function PassiveEffect(props) {
  (0, _react.useEffect)(function () {
    logger.yield("Passive effect");
  }, []);
  return _react.default.createElement(Text, {
    text: "Passive"
  });
}
yesmeck commented 5 years ago

I think /\buse[A-Z0-9]/g is enough.

DuncanMacWeb commented 5 years ago

:+1: but you don’t need the /g flag. The regex engine can stop at one match, because we’re only interested in “does this match at least once?” not “how many times does it match?”

yesmeck commented 5 years ago

you are right!