move framework related code to the framework #63
Conversation
nikgraf
force-pushed
the
ng/move-framework-code-to-the-framework
branch
from
24d22c2 to
55cdc20
Compare
nikgraf
force-pushed
the
ng/move-framework-code-to-the-framework
branch
from
d8cf2d6 to
806ddb0
Compare
| version: 9 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: "package.json" |
There was a problem hiding this comment.
there was a warning since it uses node 18 by default, since one run crashed due memory I thought let's try setting it up properly and only then debug. After the upgrade it didn't happen. Could have been a one-time error or maybe this helped :)
| automergeHandle: undefined, | ||
| }); | ||
|
|
||
| export function GraphFramework({ children, accountId }: Props) { |
There was a problem hiding this comment.
What would accountId be here?? Like the authenticated user?
There was a problem hiding this comment.
yeah, the current user
There was a problem hiding this comment.
In that case, if the user is not authenticated, can we not render the GraphFramework?
There was a problem hiding this comment.
don't know yet, haven't thought about it yet - I think the websocket connection makes only sense for authenticated users - but then maybe it doesn't have to autoconnect when including the GraphFramework in a component
| const storeState = useSelector(store, (state) => state.context); | ||
| const spaces = storeState.spaces; |
There was a problem hiding this comment.
Is it more efficient to only return the spaces here? Like:
const spaces = useSelect(store, (state) => state.context.spaces)I think xstate generally prescribes the latter as it is more efficient, better use of the selector memoization
There was a problem hiding this comment.
much better! feel free to fix it once merged 👍
There was a problem hiding this comment.
fixed it in this PR #65
| break; | ||
| } | ||
| case 'space-event': { | ||
| const space = spaces.find((s) => s.id === response.spaceId); |
There was a problem hiding this comment.
I see us doing this spaced.find pretty regularly. Would it be better to make spaces a Map<string, Space> or Record<string, Space> so we aren't having to do array lookups all the time?
There was a problem hiding this comment.
yeah, bit undecided here. In some cases the array (iterating in JSX) is really useful in some cases a record (everywhere else). What's your experience? any preferences? I was learning torwards what prisma does, but maybe something else makes more sense
There was a problem hiding this comment.
If I am constantly having to do array lookups, I use a map/record. It is easy enough to Object.values(spaceMap) to iterate through as an array.
There was a problem hiding this comment.
yeah, I do too, but multiple times heard from devs in the past that they don't like it
@pcarranzav @fubhy what's your take?
There was a problem hiding this comment.
It's impossible to answer this question without knowing what you want to optimize for. This is a data structure problem and needs to be answered based on what kind of trade offs you are willing to accept and hwat compute complexity vs. developer experience is acceptable. Do you want to optimize for read? write? memory? cpu?
Object.keys / Object.values does not create a lazy iterable. It creates a shallow array copy (so straight up copies memory for primitive values like strings, etc. and copies pointers to non-primitive values like objects). For read performance, the best is to create a data structure that creates copy-less iterables over the values / keys. That'd be a Map. And then iterating via foo.values() (creates an no-copy iterable).
There was a problem hiding this comment.
From the current structure I expect max 4 writes pers second - to update lastUpdateClock. Whenever there is a change this currently most likely will trigger a re-render in React components.
Since out aim is local first we use this as an in-memory layer for a persistence storage or remove it completely. My current thinking is to focus on the functionality and DX (with napkin math in mind like deep cloning a 3 level object is fine 4 times per second). Once we have the API & structures in place, we can optimize and if needed adept the APIs. What do you guys think?
3 participants
Moving framework related code out of the events app and into the framework.
Follow-up task will be to make sure to split it up in the framework so it works without React and React only being a thin Provider/Context layer for convenience.