-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: Structuring context & schema so it can be used on the client #17489
feat: Structuring context & schema so it can be used on the client #17489
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Great, this looks like it goes with some of the work I've been doing over in #17460 (which is more focused on actually executing things, via GraphQL). I will pull this down and go over it to fully grok it, and come at you with some questions. |
@@ -13,7 +13,7 @@ | |||
"dev": "NODE_ENV=development LAUNCHPAD=1 yarn start-test 5050 cypress:open", | |||
"postinstall": "yarn codegen && echo '@packages/launchpad needs: yarn build'", | |||
"start": "http-server -p 5050 dist", | |||
"watch": "yarn build --watch", | |||
"watch": "concurrently \"vite build --watch\" \"yarn codegen --watch\"", |
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.
Have been needing this, great
…/add-context * unified-desktop-gui: chore: fixing dev task for launchpad
wizardNavigateBack: Wizard | ||
|
||
"""Navigates forward in the wizard""" | ||
wizardNavigateForward: Wizard |
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.
We can probably simplify this to wizardNavigate(direction: forward | backward)
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.
Sure, let's get it merged and figure out the quality of life improvements - they'll be a lot easier to review in their own context. Not a blocker.
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.
wizardSetManualInstall(isManual: Boolean!): Wizard | ||
|
||
"""Sets the current testing type we want to use""" | ||
wizardSetTestingType(type: TestingTypeEnum!): Wizard |
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.
Can probably consolidate the above to wizardUpdate(input: WizardUpdate!)
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.
Can keep https://github.com/vuejs/jsx-next#syntax in mind if we want the react style early return on graphql loading pattern
@@ -0,0 +1,24 @@ | |||
import type { BaseActions } from '../actions/BaseActions' | |||
import { App, Wizard } from '../entities' | |||
import type { Project } from '../entities/Project' |
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 understand that import type
is used because at runtime no such export exists, for example in the use case of a ts
file with no runtime exports (eg, no export function
, export const or
export default), but is there any real benefit in this case (since the
Project` module does have some runtime exports)?
Happy to separate import
and import type
as a general rule - haven't looked, but assuming there is some way to lint or enforce this.
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.
but is there any real benefit in this case (since the Project` module does have some runtime exports)?
Yeah, it avoids the require
in the transpiled JS. In this case it's such a minor cost that it doesn't really matter, but you can think of circumstances where you'd want to e.g. import a type from @packages/server
without worrying about the entire code being imported executed since it potentially have side-effects. The import type
is also important if we want to be able to remove the nxs.gen
file from git since there's a sort of chicken-and-egg problem of doing a runtime import of a file that doesn't yet exist. With import type
you can sidestep that
but assuming there is some way to lint or enforce this.
Yep, this is enforced via the "importsNotUsedAsValues": "error"
which I added to the graphql
and launchpad
projects. It'll be a type-error if you're only using as a type but you don't import type
@@ -0,0 +1,3 @@ | |||
export class ContextUtils { |
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.
Do we need this?
@@ -10,19 +10,18 @@ | |||
rounded-b | |||
" | |||
> | |||
<Button @click="nextFunction()">{{ next }}</Button> | |||
<Button @click="backFunction()" variant="outline">{{ back }}</Button> | |||
<Button @click="nextFn()">{{ next }}</Button> |
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.
click
can take a callback so you can just do <Button @click="nextFn>"
, either way is fine.
As an aside, some general tips n tricks:
Another thing you may find useful is if you pass a function, it works the same as React - you can grab the args like
<Button @click="(arg1, arg2) => nextFn(arg2)" />
Additionally should you want the JS event, it's under $event
<Button @click="(arg1, arg2) => nextFn(arg2, $event)" />
back: { | ||
type: String, | ||
required: true, | ||
}, | ||
backFn: { |
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 wonder if we want some helpers, like:
const RequiredFn = {
type: Function,
required: true
} as const
// ...
props: {
nextFn: RequiredFn
}
Probably can make this improvement down the track.
Unfortunate thing is these are both runtime and compile time, so we cannot have a generic like Gql<T>
. This is an unfortunate limitation of the object syntax.
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.
Actually we should just do this: #17550
const storeApp = useStoreApp(); | ||
const storeConfig = useStoreConfig() | ||
|
||
const selectedFramework = ref<Framework | undefined>(undefined); |
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.
Love how much code is deleted here and moved to gql
Thin component is a good component
setup() { | ||
const store = useStoreApp(); | ||
const manualInstall = ref(false); | ||
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.
Here's some neat coming up. There is a new syntax called <script setup>
https://github.com/vuejs/rfcs/blob/master/active-rfcs/0040-script-setup.md
It makes vue files more concise. One of the cool things is we get type only props declarations. A bit experimental but we can write:
import { declareProps } from 'vue'
declareProps<{
gql: InstallDependenciesFragment
}>
This also solves the slow IDE plugin part (at least inside of <script>
- it is basically instant, like it should be).
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.
Consider
@@ -399,7 +396,8 @@ export class ProjectBase<TServer extends Server> extends EE { | |||
return this.initSpecStore({ specs, config: updatedConfig }) | |||
} | |||
|
|||
async initializePlugins (cfg, options) { | |||
// TODO(tim): Improve this when we completely overhaul the rest of the code here, |
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.
Yep we can nuke/rework most of this. The reason it's still the way it is, is I was trying to keep it back compatible with the current desktop-gui to make reviewing/merging more easy, but I guess we can just abandon that consider how big a change we are introducing.
* apply general improvemets from PR comments * remove unused file * comment out test for now * stub out missing story module
…ui/add-context-code-review * unified-desktop-gui: feat: Structuring context & schema so it can be used on the client (#17489) fix: vue 3 types, beta suffix & component name (#17508) fix: last test skipped with top navigation and already run suite (#17498) chore: Update Chrome (beta) to 93.0.4577.18 (#17536) fix: use process.geteuid and catch uid errors in file util (#17488) build: make vite build strict (#17509) release 8.1.0 fix(server): correctly include projectRoot when adding a CI project from GUI (#17514) fix(types): update cy.shadow docs url (#17506) chore(npm/webpack-preprocessor): fix CI pipeline (#17515) fix: test runner reporter performance (#17243) chore: release @cypress/vue-v3.0.0-beta.4 chore: release @cypress/vite-dev-server-v2.0.3 fix: make vite re-run on supportFile change (#17485)
@packages/graphql
to consolidate any GraphQL related state logic@vue/urql
cy.mountFragment
to test components w/ GraphQL fragments