refactor: Data context cleanup & IPC bindings for data push #18357

tgriesser · 2021-10-05T13:51:11Z

Summary

Adds Socket.io bindings for pushing data from the DataContext to trigger a refresh of any queries that need updating on the client. Triggered with ctx.emitter.toApp() / ctx.emitter.toLaunchpad(), will trigger a refetch of any queries currently being executed.

Additionally, does a lot of work to pass the GraphQL & App server port info to the frontend clients, which enables better e2e testing of the launchpad & app. Created DataContextShell which is the bare minimum API threaded through the open_project / ServerBase without needing a major refactor of tests - we just auto-create the DataContextShell if one isn't passed, but we will ensure one is passed from any new codepaths. This approach will provide a starting point to extract more server logic into the data-context layer.

We are testing against the dist'ed .html files, by passing the ports via the query string, otherwise the ports will be passed via injection when the HTML is rendered. This injection / testing pattern necessitates that we use hash-routing within the main app area to keep things simple.

Moves the data-context setup to makeDataContext, rather than having it in the lib/gui/events, since we're going to need to create the data context in other locations as well.

Upgrades ts-node to give us better sourcemapping when we're debugging, and adds typescript-cached-transpile to make transpilation more consistent / possible to debug between the app & plugins (since we're now using the plugins to test the app itsself, the sourcemaps need to line up).

UNIFY-413

Setting up bi-directional data communication w/ Sockets

Electron IPC for Launchpad
Socket.io layer for app
Urql exchange for auto-refetch when a signal comes in

cypress-bot · 2021-10-05T13:51:12Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

* unified-desktop-gui: chore: improving component test infra (#18378) chore(launchpad): launchpad UI tweaks (#18369) feat: wire up add project button (#18320) feat(unify): adding larger defaults for the Launchpad window (#18248) fix(app): allow launching app dev server w/o relying on `supportFile` to bundle deps (#18354) feat(unify): reporter dark theme (#18313) feat(launchpad): UNIFY-371 header menu (#18338)

* unified-desktop-gui: feat: add remove project button (#18381) fix(launchpad): prevent code colors from leaking out of reporter (#18382) Fix README on unified-desktop-gui

* unified-desktop-gui: feat(unification): adding specs list for the app (#18391) chore: merge in develop to unified-desktop-gui (#18388)

brian-mann · 2021-10-08T17:03:56Z

packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts

+
+  on('task', {
+    withCtx (fnString: string) {
+      return new Function('ctx', `return (${fnString})(ctx)`).call(undefined, ctx)


brian-mann · 2021-10-08T17:08:43Z

packages/server/lib/gui/auth.ts

  .finally(() => {
    _stopServer()
-    windows.focusMainWindow()
+    require('./windows').focusMainWindow()


reminder for me to clean this up and pass this as a callback function to avoid requiring electron

brian-mann · 2021-10-08T17:11:18Z

We also need to refactor lib/gui/events to become only a source of events, and move the handler logic to gql ctx

* unified-desktop-gui: (40 commits) feat: index.html configurability and storybook support (#18242) fix: remove .json check from require_async, prevent child_process spawn (#18416) percy snapshot the tooltip visually, prevent it from being hidden fix: failing tests from #18372 (#18414) fix: `everyNthFrame` should only be applied for Chrome 89+ (#18392) feat(app): render spec list, command log, iframe (#18372) fix: drag and drop to be correct directory (#18400) refactor: Add GitDataSource, FileDataSource, toast for errors (#18385) docs: General updates to contributing guide (#18283) Add shorter --ct alias for --component Add --e2e and --component CLI options chore: Update Chrome (beta) to 95.0.4638.40 (#18389) chore: use circleci timings split for e2e tests (#18367) fix: fixed title (#18370) chore(deps): update dependency electron to v14 🌟 (#18384) chore(server): share client route (#18215) fix: Prevent Cypress from crashing when argument parsing "spec: {}" (#18312) chore: update husky dev dependency to v7 (#18345) feat: add defineConfig function to help type config (#18302) chore: Update Chrome (stable) to 94.0.4606.71 (#18324) ...

* unified-desktop-gui: chore(unify): optimize deps automatically, clean up unused files (#18429) fixing stub types and tests (#18428) feat: expose project config to project nodes (#18424) feat(launchpad): Adding generic error screen (#18405)

* unified-desktop-gui: feat(app): persist current spec in data context, update runner workflow for demo (#18406)

tgriesser · 2021-10-11T14:25:09Z

packages/frontend-shared/src/graphql/urqlExchangePubsub.ts

+import type { Exchange, Operation } from '@urql/core'
+import type { Socket } from '@packages/socket/lib/browser'
+
+export const pubSubExchange = (io: Socket): Exchange => {


This is where the queries are watched for refresh on push signal.

Are we actually using this somewhere or not yet? It would be good to see an example of "how to push things". I'm guessing there might be one here, but there is a lot of files - might be good to include a link to the file in the PR description so everyone can easily see it.

lmiller1990

Some misc comments - I couldn't immediately see an example of where we are actually pushing something from the server here? Do we have an example of "how to do it" or not yet?

lmiller1990 · 2021-10-11T14:50:04Z

packages/data-context/src/actions/ProjectActions.ts

  }

-  clearActiveProject () {
+  async clearActiveProject () {


Silly question but can this not just be

clearActiveProject () { this.ctx.appData.activeProject = null return this.api.closeActiveProject() }

Yeah it can be, I think I might have written this way was because it was unclear what closeActiveProject is returning and didn't want to imply that you could rely on whatever the return value from that was

lmiller1990 · 2021-10-11T14:52:12Z

packages/frontend-shared/src/graphql/urqlClient.ts

 }

-export function makeUrqlClient (): Client {
+export function makeUrqlClient (target: 'launchpad' | 'app'): Client {


I think we have these typed in resolve-dist, but probably not a big deal to duplicate them. But might be good to have a single place to define these?

Maybe... for simple types like this I tend to not care about duplication too much

packages/frontend-shared/src/graphql/urqlClient.ts

lmiller1990 · 2021-10-11T14:55:51Z

packages/server/lib/gui/events.ts


-module.exports = {
+interface EventsStartArgs extends LaunchArgs {
+  onFocusTests: () => void


I think we started defining these already, full text search for export interface OpenProjectLaunchOptions, might be good to have them all in one place (ideally in @types

This is specifically extending LaunchArgs which is defined in @types with values that are local here / specific to Events.start

lmiller1990 · 2021-10-11T14:56:56Z

packages/server/lib/makeDataContext.ts

+
+const { getBrowsers } = browserUtils
+
+interface MakeDataContextOptions {


nice, I am glad this is isn't in gui/events anymore.

packages/server/lib/modes/internal-server.ts

lmiller1990 · 2021-10-11T15:06:57Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

+        msg: nonNull(stringArg()),
+      },
+      resolve: (root, args, ctx) => {
+        ctx.emitter.toLaunchpad(args.msg)


What is the purpose of having this as a mutation? Isn't the idea to be able to push things to GraphQL independantly of GraphQL (eg, via some other event/trigger)?

What is the best pattern here? Do we just grab the data context (import it however) and call emiiter.toLaunchpad, or would we be making a GraphQL mutation request to the server to access the emitter?

An example might be doing a background check to see if the git status of a file has changed via some watcher. How would we notify GraphQL that it needs to re-query to get the latest data? Woudl we make an actual request to GraphQL or just grab the context and call emitter.toApp('get:new-files') (for example)?

This was just for testing, we can delete this

An example might be doing a background check to see if the git status of a file has changed via some watcher. How would we notify GraphQL that it needs to re-query to get the latest data? Woudl we make an actual request to GraphQL or just grab the context and call emitter.toApp('get:new-files') (for example)?

Currently we just call ctx.emitter.toApp() from wherever in the background (watching, or whatever) and it'll signal the app to refetch any queries. Going for simplicitly rather than optimizing / narrowing the data fetched until it seems necessary due to perf or similar.

data context cleanup & IPC bindings for data push

5131b37

tgriesser requested a review from a team as a code owner October 5, 2021 13:51

tgriesser requested review from BlueWinds and jennifer-shehane and removed request for a team, BlueWinds and jennifer-shehane October 5, 2021 13:51

tgriesser marked this pull request as draft October 5, 2021 13:51

tgriesser changed the title ~~data context cleanup & IPC bindings for data push~~ Data context cleanup & IPC bindings for data push Oct 5, 2021

tgriesser added 3 commits October 6, 2021 14:31

Move cors to server

5393e71

Close graphql server

757d48e

tgriesser added the component testing label Oct 6, 2021

tgriesser added 6 commits October 6, 2021 15:52

Fixes

7a5eb03

Cors as a server dep

6e3acff

Merge branch 'unified-desktop-gui' into tgriesser/unify/ui-data-push

3abb5af

* unified-desktop-gui: feat: add remove project button (#18381) fix(launchpad): prevent code colors from leaking out of reporter (#18382) Fix README on unified-desktop-gui

[wip] pairing w/ Brian to cleanup test mode

703cb61

update

2b64997

WIP on sockets / data-context / server / testing overhaul

18cc057

tgriesser mentioned this pull request Oct 7, 2021

[WIP] Cleanup of e2e Testing for Unified GUI #18356

Closed

4 tasks

Merge branch 'unified-desktop-gui' into tgriesser/unify/ui-data-push

eaa6f93

* unified-desktop-gui: feat(unification): adding specs list for the app (#18391) chore: merge in develop to unified-desktop-gui (#18388)

brian-mann reviewed Oct 8, 2021

View reviewed changes

tgriesser added 5 commits October 8, 2021 15:01

tsc cleanup, more work on e2e & socket layer

0c5eee1

Remove outdated patch-package

4eb8487

noUnusedLocals false, should be caught with linting

d8a9068

Fixes

f3d4cc7

tgriesser changed the title ~~Data context cleanup & IPC bindings for data push~~ refactor: Data context cleanup & IPC bindings for data push Oct 10, 2021

tgriesser added 13 commits October 10, 2021 20:15

Fix type

160c1ec

Fix type

1f7f259

improved script cleaning

292d663

Don't export client from @packages/socket

5b05c4c

add packages to devDeps

0422061

Fix

b5d0ab7

add src to files

3690aec

Fix unit test

1da2ee4

cleanup __CYPRESS_GRAPHQL_PORT injection

f13c63e

fix runner tests

69449f2

Merge branch 'unified-desktop-gui' into tgriesser/unify/ui-data-push

7903bc5

* unified-desktop-gui: feat(app): persist current spec in data context, update runner workflow for demo (#18406)

fixes

7ab2dff

fix another mock

4d92778

tgriesser requested a review from brian-mann October 11, 2021 04:07

Remove unused --manifest

4434925

tgriesser commented Oct 11, 2021

View reviewed changes

tgriesser requested a review from lmiller1990 October 11, 2021 14:31

tgriesser marked this pull request as ready for review October 11, 2021 14:33

tgriesser requested a review from ImCesar October 11, 2021 14:33

lmiller1990 reviewed Oct 11, 2021

View reviewed changes

Move SUPPORTED_PLATFORMS to @packages/types, add clarifying TODOs

7f51d51

tgriesser merged commit d841e13 into unified-desktop-gui Oct 11, 2021

tgriesser deleted the tgriesser/unify/ui-data-push branch October 11, 2021 16:13


		const { getBrowsers } = browserUtils

		interface MakeDataContextOptions {

refactor: Data context cleanup & IPC bindings for data push #18357

refactor: Data context cleanup & IPC bindings for data push #18357

Uh oh!

Conversation

tgriesser commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

cypress-bot bot commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-mann commented Oct 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgriesser Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgriesser commented Oct 5, 2021 •

edited

Loading

tgriesser Oct 11, 2021 •

edited

Loading