Compare "Other Keys" branch with initial blank commit #6
Conversation
* horizontal buttons with chord names * you can add chords to draft view * add playwright * add playwright tests * fix GH workflow artifact upload * fix GH workflow artifact upload #2 * add code coverage * fix workflow file * fix workflow run in background * hardcode not running server in playwright process * fix workflow for coverage post * disable code coverage report in CI * code coverage continue-on-error * comment out code coverage in workflow. would rather it be obvious it's "not working" * code coverage upload artifact * explicitly stop running server in actions workflow * use ts-node with nyc * add server logs to workflow artifacts * fix stop server pid in workflow * avoid playwright web server in ci * change wait pid to sleep * try other sleep tactic * make it so playwright video is always recorded * fix test server path * change "npm start" to run compiled build * add "new jam" button * websocket update works
* add linter * run lint * add ci workflow for lint checks etc. * rename ci job
server/src/modules/jam.tsx
Outdated
| export const initJamRouterWebsocket = () => { | ||
| jamRouter.ws(ROUTES.JAM_ROOM_WEBSOCKET, (ws, req) => { | ||
| connectedSockets.push(ws); | ||
| console.log('ws connected'); | ||
| ws.send(JamView().toString()); |
There was a problem hiding this comment.
All the main stuff is pretty much "jammed" 🎸 into this file right now. Planning on separating this into a separate structure like:
modules/jam/jam_controller.tsmodules/jam/jam_views.tsx
| return ( | ||
| <div | ||
| id='jam-view' | ||
| hx-ws='message:replaceOuterHTML' |
There was a problem hiding this comment.
Also want to abstract out any things like this. I actually don't think we should use hx-ws here to dictate where the response will go.
The view should not have to "know" what is going to be updated with the websocket message. Instead the websocket messages themselves should contain "out of band" swapping instructions https://htmx.org/extensions/web-sockets/#receiving-messages-from-a-websocket
| hx-post={`${ | ||
| ROUTES.JAM_ACTIONS_ADD_CHORD | ||
| }?${params.toString()}`} |
There was a problem hiding this comment.
This is disgusting, and exactly why I suggest we use functions passed into views to avoid this exact sort of thing, in the comment below
| export const JamPage = () => { | ||
| return ( | ||
| <> | ||
| <div hx-ws='connect:/jam/ws' /> |
There was a problem hiding this comment.
Also want to decouple the URL structure away from the views/components. There should be a functions like:
const makeJamViewWebsocketUrl = () => `connect:/jam/ws`;
const makeAddChordUrl = (chord: string) => `/jam/add_chord?chord=${chord}`;I think all URL things should be done with functions like these, so there is no knowledge of the URL themselves in the view. Much easier to refactor. They are essentially onClick props, in the form of URL generator functions.
These functions will be defined right next to the handlers that process the requests for that URL. They can use some other defined enum, but I prefer seeing the URL as a magic string when looking at the handler. No other code needs to know the URL structure, as that other code will just use the makeAddChordUrl function etc.
Also, no more onChange handlers because we're not using React 🙂 We let the browser natively manage its form state
This PR is meant to compare the open PR #4 with the initial commit of the repository, so the whole repository can be reviewed at once here.
Feel free to take a look at previous PRs in isolation: