-
Notifications
You must be signed in to change notification settings - Fork 8
move framework related code to the framework #63
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
Conversation
24d22c2
to
55cdc20
Compare
d8cf2d6
to
806ddb0
Compare
version: 9 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version-file: "package.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely like this direction 🔥. I still want to make the react layer its own, separate package. But I think this is a great path
automergeHandle: undefined, | ||
}); | ||
|
||
export function GraphFramework({ children, accountId }: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would accountId
be here?? Like the authenticated user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the current user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, if the user is not authenticated, can we not render the GraphFramework
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better! feel free to fix it once merged 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.