-
Notifications
You must be signed in to change notification settings - Fork 72
feat: Playground ACP #3871
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: Playground ACP #3871
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3871 +/- ##
===========================================
- Coverage 75.33% 75.29% -0.04%
===========================================
Files 434 434
Lines 40378 40378
===========================================
- Hits 30415 30400 -15
- Misses 7964 7976 +12
- Partials 1999 2002 +3
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
AndrewSisley
left a comment
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.
The feature itself is fantastic (going by your demo a little while ago), but I have some concerns about the maintainability of the code - I am not sure how serious we want to take development of the playground - If we are serious about this, I think it needs some refactoring, and tests, before we should merge this. I'm going to raise this in discord I think.
| import { defineConfig, loadEnv } from 'vite' | ||
| import react from '@vitejs/plugin-react-swc' | ||
|
|
||
| // https://vitejs.dev/config/ |
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.
question: Why the removal of the link to the documentation?
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.
addressed, thanks
| <button | ||
| onClick={handleAddRelationship} | ||
| disabled={isAddLoading} | ||
| style={{ |
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.
question: This is quite a lot of css, and quite a lot of it looks like it might be duplicated, and it is duplicated across stuff where consistency should really be enforced.
I don't know th playground, but I worked with react for a few years, and it looks like these are just react components - did you look into best practices for this kind of thing? Can we refactor this slightly? Can we use a proper component heirachy using styled components, or do you think that is overkill atm https://styled-components.com/ ?
I worry that maintaining the css in these files would not be be very fun.
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.
@AndrewSisley you're right, the initial pass of the styling is less than ideal. Refactored it as well as other plugins
| ).catch(err => { | ||
| console.error("Error loading Wasm module:", err); | ||
| }) | ||
| (window as any).globalACPConfig = { |
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.
question: Has been a long time since I've done any serious web dev, but I remember that window is not a great place to keep stuff (edit: I see thats also where we keep the defra instance atm though). I also can't spot where this is read. Is this just set for debugging reasons?
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.
For the defra instance, it's exposed this way in the playground specifically so that devs can try out defra commands in the console.
playground/src/App.tsx
Outdated
| if (mode === 'wasm') { | ||
| if (!isClientReady) { | ||
| return ( | ||
| <div style={{ |
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.
todo: Please document why this particular css is returned, if a refactor does not make this more obvious.
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.
@AndrewSisley changed to return empty fragment instead of stylized div
| } finally { | ||
| setIsDeleting(false); | ||
| } | ||
| window.location.reload(); |
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.
question: Why is the entire page being reloaded 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.
that was a quick workaround to reinstantiate DefraDB client after resetting the keypair since the new keypair is created on "open()"
| setIsDeleting(true); | ||
| setResult('Deleting keypair...'); | ||
| try { | ||
| const acpDeleteKeypair = (window as any).acp_DeleteKeypair; |
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.
question: I can't see where acp_DeleteKeypair is set, can you maybe document what it is and where it came from? And why it is directly in window?
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.
@AndrewSisley it is one of the global bridge functions defined globally within the acp-js library. so that we can access and them from the sourcehub acp client in go
| clientRef: React.RefObject<any>; | ||
| } | ||
|
|
||
| export const createKeypairPlugin = (props: KeypairPluginProps): GraphiQLPlugin => ({ |
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.
todo: The name of this component is very generic, and gives little hint as to what it is used for, however reading the implementation it appears to be very tightly coupled to ACP. Can you please rename this, and add documentation.
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.
thanks, addressed!
| onClick={() => { | ||
| const textarea = document.getElementById('policy-input') as HTMLTextAreaElement; | ||
| const button = document.getElementById('add-policy-button') as HTMLButtonElement; | ||
| const resultDiv = document.getElementById('policy-result'); |
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.
suggestion: The stuff in this onClick function is surprising to see in a react element, and I believe would usually be handled by the react component system, and styled components, instead of raw JS stuff.
I suggest reworking these elements/functions to avoid this, but that perhaps depends on how seriously we want to take the development of the playground.
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.
@AndrewSisley 💯 agree, thanks. Reworking it
david-on-github
left a comment
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.
Nice work. General recommendations for the next iteration:
- Add a state store (zustand)
- Create ui components for commonly used elements. eg. button, forms. (I personally like shadcn and tailwind as a ui library since it reduces a lot of the manual setup for interactive accessibility, etc and centralizes styles with css variables)
- Update / Look at alternatives for the incompatible/outdated dependancies if it's not too much effort
playground/src/App.tsx
Outdated
| schemaRef.current = DEFAULT_SCHEMA; | ||
| }, [DEFAULT_SCHEMA]); | ||
|
|
||
| useEffect(() => { |
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.
These are constants, so this effect appears redundant.
playground/src/App.tsx
Outdated
| console.error('Failed to initialize DefraDB Wasm client:', error); | ||
| } | ||
| }; | ||
| initClient(); |
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 don't have much context. But could loading the wasm module could be moved here and awaited instead of polling to see if it was eventually loaded?
/ Minor, but there should be a cleanup function here that clears the timeout.
playground/src/App.tsx
Outdated
| fetcher={wasmFetcher} | ||
| plugins={[ | ||
| ...(isSourceHubAvailable ? [keypairResetPlugin] : []), | ||
| policyTogglePlugin, |
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.
As discussed - after some investigation to maintain state across the app and inside the plugin windows without the windows disappearing, better to make these plugins static exports and use zustand as a store inside the plugin components to manage the state and actions.
playground/src/App.tsx
Outdated
| // All operations go through execRequest. | ||
| const result = await client.execRequest(query, args); | ||
| return result.gql; | ||
| if (initRef.current) { |
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.
This applies generally, but likely cleaner to move actions like initClient to a state store and have flags for the statuses and results. 'loading / ready / error / etc'
| const handlePolicyChange = useCallback((value: string) => { | ||
| setPolicyText(value); | ||
| props.policyRef.current = value; | ||
| }, [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.
Adding 'props' as a dep here would make the useCallback hook redundant.
I see other places this is done too. It would add overhead since it gets redefined on each render.
| useZeroFees: import.meta.env.VITE_ACP_ALLOW_ZERO_FEES === 'true' || false, | ||
| }; | ||
|
|
||
| await instantiate('defradb.wasm'); |
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.
thought: It's a little odd that Defra is instantiated from the acp package.
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.
@fredcarle it is part of the wasm bridge of the acp-js library, which indeed looks a bit odd. The main goal of placing it there was to simplify the client side setup logic (e.g. linking wasm_exec.js).
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.
@iverc fwiw If this call fails for any reason the app won't start.
playground/README.md
Outdated
| npm install | ||
| npm run dev:remote | ||
| ``` | ||
| window.defradbClient.addDACPolicy(policy, context, acpConfig); |
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.
thought: I don't think acpConfig is a valid param for addDACPolicy.
| @@ -0,0 +1,11 @@ | |||
| # DefraDB ACP Configuration | |||
| VITE_ACP_API_URL=/api | |||
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.
question: Is the VITE prefix required?
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.
@fredcarle yes, otherwise they're not accessible on the FE side
AndrewSisley
left a comment
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.
Approving now, I'm happy so long as the others are (please make sure you resolve any of their comments before merge) - looks like you've done a very good job at cleaning up my core concerns in a very short period of time.
Thanks Ivan :)
david-on-github
left a comment
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.
lgtm, super clean
playground/src/App.tsx
Outdated
|
|
||
| if (!client) { | ||
| return <></>; | ||
| const wasmFetcher: Fetcher = useCallback(async (graphQLParams: any) => { |
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.
[nit] This doesn't seem to consume any props, so could be extracted to a standalone function.
playground/src/App.tsx
Outdated
| ); | ||
| } else { | ||
| const baseUrl = import.meta.env.DEV ? 'http://localhost:9181' : ''; | ||
| const SwaggerUI = React.lazy(() => import('swagger-ui-react')); |
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.
[nit] This could be pulled out into a swagger plugin file.
And then the GraphiQL props for modes (fetcher, plugins) could be extracted into a hook, and only a single GraphiQL component would need to be returned out of App.
|
Bug bash result:
|
Relevant issue(s)
Resolves #3870
Description
This PR adds comprehensive ACP functionality to the Playground with support for both Local ACP and SourceHub ACP clients:
Tasks
How has this been tested?
Tested both ACP modes:
.envsetup and routing to SourceHub endpoints.Specify the platform(s) on which this was tested: