purescript-react / purescript-react-basic-hooks

An implementation of React hooks on top of purescript-react-basic
https://pursuit.purescript.org/packages/purescript-react-basic-hooks/
Apache License 2.0
200 stars 33 forks source link

Using exported components in an existing JS/TS React project #4

Closed muratg closed 5 years ago

muratg commented 5 years ago

Is there a way to make this work? React project build system complains about rules of hooks being broken.

Line 658: React Hook "React.useState" is called in function "exports.useState_" which is neither a React function component or a custom React Hook function react-hooks/rules-of-hooks

This is with a simple component:

mkCounter :: CreateComponent {}
mkCounter = do
  component "Counter" \props -> React.do
    counter /\ setCounter <- useState 0

    -- useEffect counter do
    --   setDocumentTitle $ "Count: " <> show counter
    --   pure mempty

    pure $ fragment
      [ R.h1_ [ R.text $ "New counter!" ]
      , R.label_ [ R.text $ "Hook counter... " <> show counter]
      , R.hr {}
      , R.button
          { onClick: handler_ $ setCounter (_ + 1)
          , children: [ R.text $ "Increment "  ]
          , className: "btn btn-primary"
          }
      , R.button
          { onClick: handler_ $ setCounter (_ - 1)
          , children: [ R.text $ "Decrement" ]
          , className: "btn btn-secondary"
          }
      ]

My goal is to write most of my app with TypeScript, but implement some of the features with PureScript to see if it works for me.

I was able to make react-basic components work in the same TypeScript app, but I'd rather use hooks.

(Thanks for the great lib, BTW!)

megamaddu commented 5 years ago

How are you using this component in JS/TS?

muratg commented 5 years ago
  1. Bundle it as a module with spago bundle-module -m Counter -t counter.js
  2. Import it from my TypeScript code with const Counter = require('../purs/counter.js').mkCounter

At this point the watch process errors out with this part of counter.js, specifically React.useState bit:

(function(exports) {
  "use strict";
  var React =require("react");

  exports.useState_ = function(tuple, initialState) {
    var r = React.useState(initialState);
    var state = r[0];
    var setState = r[1];
    return tuple(state, function(update) {
      return function() {
        return setState(update);
      };
    });
  };

  exports.unsafeSetDisplayName = function(displayName, component) {
    component.displayName = displayName;
    component.toString = function() {
      return displayName;
    };
    return component;
  };

  exports.displayName = function(component) {
    return typeof component === "string"
      ? component
      : component.displayName || "[unknown]";
  };
})(PS["React.Basic.Hooks"] = PS["React.Basic.Hooks"] || {});

(I may totally be holding it wrong BTW, but I was able to get a react-basic component work with the same workflow.)

megamaddu commented 5 years ago

Can I see the code where you actually import and render mkCounter? It needs to run the CreateComponent effect, i.e.:

import { mkCounter } from '...';

const Counter = mkCounter();

...
  <Counter />
...
muratg commented 5 years ago

This line alone causes it to complain: const Counter = require('../purs/counter.js'). This is without even trying to use the component.

I converted it to use an import statement to see if it works:

import * as counter from '../purs/counter.js'

const Help: React.FC = () => {
  const Counter = counter.mkCounter()
  useDocumentTitle('Help')
  return (
    <div className='Help'>
      <Counter label='test' helpText='what up dude' />
...

... nope. Same error.

Could it be executing the effect while importing/requiring the file?

muratg commented 5 years ago

Note: I created the app with create-react-app --typescript. Perhaps it's an overly aggressive linter rule. :thinking:

megamaddu commented 5 years ago

Do hooks work in your TS components? It seems like setup somehow but I'm not sure.. can I clone it?

Also, the component creation effect should only happen once at the module level, so you'll want to move that line out of your Help component.

muratg commented 5 years ago

Yeah, I use state and context hooks as well as effects. (Even have a custom hook to set the title.)

This is an internal app, but I'll create a simple project that reproduces the issue after lunch and share it with you.

(Normally that line is at the module level, I was just testing to run it "inside a component" to see if that would change anything.)

muratg commented 5 years ago

Here is a repro app: https://github.com/muratg/hooksrepro

It has three versions of the counter, TypeScript, react-basic, and react-basic-hooks. (The latter is commented out to enable the build.)

megamaddu commented 5 years ago

Thanks, I'll take a look soon

megamaddu commented 5 years ago

@muratg I'm having trouble understanding how this is set up. I see two PS files, but nothing references them. How are you compiling them?

muratg commented 5 years ago

@spicydonuts the project was created with create-react-app. The PureScript files were included only for your info in that project, they are not used directly. The .js files are.

In case it helps, I shared the project which I used to generate the .js files here: https://github.com/muratg/hooksrepro_libs. You can use the relevant npm script to create either file.

Does this help?

megamaddu commented 5 years ago

I'm not quite sure why those bundles aren't working yet, but you could try starting or copying from https://github.com/justinwoo/spacchetti-react-basic-starter/ in the meantime

muratg commented 5 years ago

Thanks! CounterClass.purs in my repro project is similar to what exists in that repo, and it works -- I can use react-basic just fine in my setup, just not react-basic-hooks in the same setup.

My TypeScript project is fully functional components and hooks based (feels much nicer than creating component classes!) So I was hoping to be able to do the same in PureScript parts.

I'll keep looking when I have some free time, perhaps it's something simple that breaks the interaction between the common.js bundle and React rules.

megamaddu commented 5 years ago

I thought at first there might be two copies of React bundled, but that doesn't seem to be the case.

muratg commented 5 years ago

I found the problem :tada: Fixed the symptom in my repro with this commit. See the change in counterHooks.js.

With this change, the error in my first comment above is gone and the component works.

I'm guessing React is having difficulty to identify the custom hook when it's created with exports.useXyz = func... but is fine when it's created with var useXyz = func... and then exported with exports.useXyz = useXyz.

The fix seems simple enough. I'm happy to send a PR if you're interested!

megamaddu commented 5 years ago

What do you mean by "identify the custom hook"? It's really strange that this fixes it.

muratg commented 5 years ago

Well, the error in the first place was Line 658: React Hook "React.useState" is called in function "exports.useState_" which is neither a React function component or a custom React Hook function react-hooks/rules-of-hooks.

This suggests that React doesn't think exports.useState_ is a custom hook.

Making the change made this error go away.

megamaddu commented 5 years ago

That just means the function was called outside of a React function component. You'd get the same error if you called CounterTS({}) outside of a React component. I think something is finding and calling that function before it should, possibly some create-react-app tooling?

megamaddu commented 5 years ago

Oh yes, it's a linting error, not a JS build or runtime error, from the rule react-hooks/rules-of-hook.

muratg commented 5 years ago

I think you're right that create-react-app is involved -- I believe it checks the build output for "rules of hooks".

megamaddu commented 5 years ago

Normally the compiled PS is in the folder output, so you could probably add an exception for that folder to skip eslint entirely.

megamaddu commented 5 years ago

Ok, going to close this then. Feel free to reopen if I've missed something.

muratg commented 5 years ago

Sure. I'll workaround the issue for now.

muratg commented 5 years ago

One more data point, in case you're interested:

The following also works (Notice how I name the function, instead of keeping it anonymous):

  exports.useState_ = function useState_(tuple, initialState) {
    var r = React.useState(initialState);
    var state = r[0];
    var setState = r[1];
    return tuple(state, function(update) {
      return function() {
        return setState(update);
      };
    });
  };

This shuts up the eslint rule.

Would you consider this change in the library?

megamaddu commented 5 years ago

I don't think so. Compiled output isn't supposed to be linted, since it isn't supposed to be edited. This library also has no control over every user's eslint rules, so there isn't even a "correct" version to use.