Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hooks interface #11

Closed
zemccartney opened this issue Jun 15, 2020 · 6 comments
Closed

Add hooks interface #11

zemccartney opened this issue Jun 15, 2020 · 6 comments
Assignees
Labels
breaking changes enhancement New feature or request
Milestone

Comments

@zemccartney
Copy link
Contributor

zemccartney commented Jun 15, 2020

note: words here written by Devin Ivy, from original specification of work

In order to utilize react hooks on our next project, strange-middle-end will need a hooks interface. In short, we would like to be able to get a hold of the middle-end instance within a component:

function MyComponent() {
    const m = useMiddleEnd();
    // ...
}

To achieve this we basically just need a middle-end-"branded" wrapper around react contexts and useContext(). There is an in-app example of this in fishbowl: https://github.com/devinivy/fishbowl/blob/master/packages/frontend/src/middle-end/react.js

It would be nice to take it a step further, though, and give the context provider a display name and perhaps use a prop other than value on the context provider. It would also be nice to have some test coverage.

@zemccartney zemccartney added the enhancement New feature or request label Jun 15, 2020
@zemccartney
Copy link
Contributor Author

@devinivy @tylerconstance yo! I've wondered about the following as I've worked through this. What do y'all think?

  • How do we feel about breaking this project / any other React bindings for middle-end into a separate repo e.g redux is to react-redux?
    • Because I've never worked w/ Jest before / am generally overwhelmed by frontend tooling still 😛 I've been working on this in a separate repo anyway. I could see the React-specific deps and files e.g. jest-config, cluttering this repo a bit. I could also see collocating everything massively simplifying review and management of all the work. I was planning on folding my work back into this repo once I've got my head more around it anyway, so I'll stick with that plan unless we opt otherwise
  • How tightly coupled to react-redux do we want to be? Should strange-middle-end's react bindings encapsulate react-redux entirely
    • It seems we generally assume and want it to be the case that our ReactRedux.Provider's store is the one created and managed by our middle-end
    • useMiddleEnd effectively replaces all of react-redux's hooks except for useSelector
      • useStore ---> m.store
      • useDispatch ---> m.dispatch
    • So, the only parts of react-redux's API users need to see right now are the Provider — possibly unnecessarily, as this is entirely to sync with our middle-end — and useSelector b/c we don't yet have a way to subscribe to state changes
    • Do we want to take steps to hide both behind strange-middle-end's hooks interface e.g. enclose react-redux's Provider in ours, maybe provide our own useSelector hook that somehow accesses our store's selectors?
  • Should we require users to initialize their middle-end prior to passing to our Provider? Any use case for initializing after rendering? If so, how do we want to allow said initialization? How do we want to notify dependents of the middle-end updating?
  • What should name of value passed to provider be?
    • I went with app for now, but totally easy to change to whatever
  • What should displayName be?
    • I went with StrangeMiddleEnd for now
  • Do we want to conditionalize setting displayName on NODE_ENV?

@devinivy
Copy link
Contributor

devinivy commented Jun 15, 2020

I have some cents @zemccartney! Down to ping-pong it a bit more if this feels incomplete or rushed to you.

How do we feel about breaking this project / any other React bindings for middle-end into a separate repo e.g redux is to react-redux?

I lean heavily towards keeping it in this project. I don't think you'll necessarily need jest to test it out, but the react testing library still could be useful: https://testing-library.com/docs/react-testing-library/setup#using-without-jest. I recommend still testing it in node, as though it's server-side rendered basically. You should still be able to do all the relevant react stuff without a browser. You also don't need JSX— consider calling React.createElement() directly or if you really aren't enjoying that, then pulling in something like htm. I would recommend against introducing a build step for the tests.

How tightly coupled to react-redux do we want to be? Should strange-middle-end's react bindings encapsulate react-redux entirely

In my opinion these should just be two separate things, totally decoupled. The usage should end-up almost identical to what you see in fishbowl for now: the user just adds one provider for the middle-end, and another provider for react-redux. And yeah, useSelector() and useMiddleEnd() can be expected to be used together for sure 👍

Should we require users to initialize their middle-end prior to passing to our Provider? Any use case for initializing after rendering? If so, how do we want to allow said initialization? How do we want to notify dependents of the middle-end updating?

Good point. I can't think of any reason you'd want to provide an uninitialized middle-end, so feel free to make an assertion that it is initialized.

What should name of value passed to provider be?

The name app sounds good to me, although I have run into some ambiguity going down that path since app also seems to be a common name for the mod that controls generic application information (so you might end-up writing something like app.dispatch.app.someAction()). I would have no issues with something explicit like middleEnd. Overall your call!

What should displayName be?

If you're talking about the displayName of the context itself, that sounds good to me!

Do we want to conditionalize setting displayName on NODE_ENV?

It would be a nice-to-have, but I'm not super worried about those few bytes.

@zemccartney
Copy link
Contributor Author

@devinivy hey, thanks for the super-detailed response, that cleared up just about everything. I'm sure I'll confuse myself with setting up the test environment, but I'll get something running, push it up, and revise from there.

feel free to make an assertion that it is initialized.

Cool, will do! Out of scope here, I think, but on reviewing fishbowl, particular this mod initializer, I began wondering: do we need to handle async initializers specially at all? Specifically, could there be a case where the app reads as initialized, but a mod with an async initializer hasn't actually finished initializing prior to something e.g. component-level usage, trying to reference some functionality in that mod that depends on the resolution of that async initializer, leading to...some maybe bad thing, depending on implementation details? Dunno if I phrased that clearly 🎺 And dunno if that's a realistic issue, more paranoia. To be clear, this question is sparked by the fact that I'd never even considered an async initializer before, realized I'd been thinking too narrowly about what you can do with strange-middle-end :)

If you're talking about the displayName of the context itself

Sure was! 🤦 Thanks for translating 😄

@devinivy
Copy link
Contributor

devinivy commented Jun 15, 2020

Feel free to create a separate issue about async initializers if you'd like to think that through together. Right now we're in the business of adding support for hooks without changing any other behavior of the library, but that doesn't preclude making other changes later if desired. To clarify the current behavior, the app does always become initialized synchronously, so m.initialize() wont await async initializers at all.

@zemccartney
Copy link
Contributor Author

Totally, all sounds good. I'll follow through on that after working through this hooks setup. Thanks!

@zemccartney
Copy link
Contributor Author

Released! https://www.npmjs.com/package/strange-middle-end/v/0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants