Optimize calendar performance and fix bugs #93
Conversation
✅ Deploy Preview for boisterous-sunburst-a5c941 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
georgegevoian
changed the title
georgegevoian
force-pushed
the
calendar-performance-optimizations
branch
from
bb009f3
to
ff78779
Compare
georgegevoian
changed the title
.github/workflows/main.yml
Outdated
There was a problem hiding this comment.
Just syncing the Node version with the main Grist app here.
| @@ -12,26 +12,22 @@ | |||
| "test": "docker image inspect gristlabs/grist --format 'gristlabs/grist image present' && NODE_PATH=_build SELENIUM_BROWSER=chrome mocha -g \"${GREP_TESTS}\" _build/test/*.js", | |||
| "test:ci": "MOCHA_WEBDRIVER_HEADLESS=1 npm run test", | |||
| "pretest": "docker pull gristlabs/grist && tsc --build && node ./buildtools/publish.js http://localhost:9998", | |||
| "grist": "docker run -t -i --rm --name grist-dev --network=host -e PORT=8484 -e GRIST_SINGLE_ORG=preview -e GRIST_WIDGET_LIST_URL=http://localhost:8585/manifest.json gristlabs/grist" | |||
There was a problem hiding this comment.
Removing --network=host here and elsewhere because it's unsupported on macOS.
There was a problem hiding this comment.
What is this supposed to do? I don't see any exposed port to connect to? Also what is host-gateway used for?
Once I figure out how this is supposed to be used, I'd like to double check it, since in the past host.docker.internal worked differently on macs vs linux (hopefully that's all fixed long ago, but I'd like to double check)
There was a problem hiding this comment.
That's a good point - I'm looking now and this script looks unused. May be ok to remove?
There was a problem hiding this comment.
I think I added it, so that it is easy to just build widgets and test them manually. Fine with removing it, or just leaving it as it is for linux users :).
(as this uses host network, docker container is just available on localhost:8484, there is no network isolation, so no ports forwarding)
| @@ -12,26 +12,22 @@ | |||
| "test": "docker image inspect gristlabs/grist --format 'gristlabs/grist image present' && NODE_PATH=_build SELENIUM_BROWSER=chrome mocha -g \"${GREP_TESTS}\" _build/test/*.js", | |||
| "test:ci": "MOCHA_WEBDRIVER_HEADLESS=1 npm run test", | |||
| "pretest": "docker pull gristlabs/grist && tsc --build && node ./buildtools/publish.js http://localhost:9998", | |||
| "grist": "docker run -t -i --rm --name grist-dev --network=host -e PORT=8484 -e GRIST_SINGLE_ORG=preview -e GRIST_WIDGET_LIST_URL=http://localhost:8585/manifest.json gristlabs/grist" | |||
There was a problem hiding this comment.
What is this supposed to do? I don't see any exposed port to connect to? Also what is host-gateway used for?
Once I figure out how this is supposed to be used, I'd like to double check it, since in the past host.docker.internal worked differently on macs vs linux (hopefully that's all fixed long ago, but I'd like to double check)
| @@ -63,10 +63,10 @@ export class GristTestServer { | |||
| await this.stop(); | |||
| const {gristContainerName, gristImage, gristPort, contentPort} = serverSettings; | |||
| const cmd = `docker run -d --rm --name ${gristContainerName}` + | |||
| ` --network="host"` + | |||
| ' --add-host=host.docker.internal:host-gateway' + | |||
There was a problem hiding this comment.
What role does host-gateway play?
There was a problem hiding this comment.
I don't have a Linux machine to test with (though I will go ahead and remove the flag and re-run the CI to check), but I believe this is required for host.docker.internal to work on Linux.
There was a problem hiding this comment.
What do you think about checking the OS before building the docker command, and only applying these changes if it's not Linux? If I understand the situation right on Windows, it has the same limitation as macOS.
| @@ -117,7 +117,8 @@ export class GristTestServer { | |||
|
|
|||
| public get assetUrl() { | |||
| const {contentPort} = serverSettings; | |||
| return `http://localhost:${contentPort}`; | |||
| // localhost doesn't work on Node 18 (see https://github.com/node-fetch/node-fetch/issues/1624) | |||
There was a problem hiding this comment.
That's a bit scary. I see it is ipv6 related - node-fetch/node-fetch#1624 (comment)
Does this have any consequences for the move to node 18 in Grist? I feel like there's a lot of dependence on localhost in test machinery?
There was a problem hiding this comment.
I don't think we make any outbound requests to localhost from Node, and the failures I've seen since the upgrade feel more symptomatic of general flakiness.
I see one option is to do dns.setDefaultResultOrder('ipv4first') to go back to the pre-17 behavior.
There was a problem hiding this comment.
I wonder about grist-core, grist-omnibus, maybe grist-electron. Grist does make some internal API calls, and APP_HOME_URL could easily end up being based on localhost. When docker is involved, localhost would have been a different host, but I feel like I remember some stuff in grist-omnibus working around this.
Anyway, that's all separate to optimizing calendar performance :)
There was a problem hiding this comment.
Yeah. I suspect that all may still be working. Even in this file, the fetch to localhost works fine for the Grist instance running in Docker - it's just live-server that is having the ipv6 problem. We should take a closer look after.
georgegevoian
force-pushed
the
calendar-performance-optimizations
branch
from
16fcaba
to
1874713
Compare
No description provided.