-
Notifications
You must be signed in to change notification settings - Fork 1
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
Comments
@devinivy @tylerconstance yo! I've wondered about the following as I've worked through this. What do y'all think?
|
I have some cents @zemccartney! Down to ping-pong it a bit more if this feels incomplete or rushed to you.
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
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,
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.
The name
If you're talking about the displayName of the context itself, that sounds good to me!
It would be a nice-to-have, but I'm not super worried about those few bytes. |
@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.
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 :)
Sure was! 🤦 Thanks for translating 😄 |
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 |
Totally, all sounds good. I'll follow through on that after working through this hooks setup. Thanks! |
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:
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.jsIt 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.The text was updated successfully, but these errors were encountered: