-
Notifications
You must be signed in to change notification settings - Fork 72
feat: Add WASM support for Playground #3813
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3813 +/- ##
===========================================
- Coverage 76.36% 76.27% -0.09%
===========================================
Files 425 425
Lines 39301 39301
===========================================
- Hits 30012 29976 -36
- Misses 7375 7399 +24
- Partials 1914 1926 +12
Flags with carried forward coverage won't be shown. Click here to find out more. see 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
fredcarle
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.
Thanks for bringing in this feature to the playground Ivan! I have a couple todos that I would like to resolve before approval.
playground/src/App.tsx
Outdated
| if (initRef.current) { | ||
| return; | ||
| } | ||
| // @ts-expect-error - window.defradb is set in main_js.go. |
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: Please add the full path to remove potential confusion cmd/defradb/main_js.go
playground/src/App.tsx
Outdated
| const query = graphQLParams.query || ''; | ||
| const variables = graphQLParams.variables || {}; | ||
| // All operations go through execRequest. | ||
| const result = await client.execRequest(query, JSON.stringify(variables)); |
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 execRequest method expects the query and a set of arguments. So not a stringified object. The request can also handle operationName. In its current form, passing in variables doesn't work. This is how I got it to work properly:
const query = graphQLParams.query || '';
const variables = graphQLParams.variables || {};
const operationName = graphQLParams.operationName || {};
const args = {
operationName,
variables,
};
// All operations go through execRequest.
const result = await client.execRequest(query, args);Feel free to modify this if you prefer an different format.
| cd playground | ||
| npm install | ||
| npm run dev: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.
todo: Please add information about the fact that the client is available in the console through the window.defradbClient object.
fredcarle
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. Thanks for making the changes.
playground/src/App.tsx
Outdated
| const args = { | ||
| operationName, | ||
| variables, | ||
| }; |
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.
nitpick: The indentation seems off here.
|
Bug bash result:
|
Relevant issue(s)
Resolves #3812
Description
This PR adds WASM support to the playground. Now there are two modes: wasm (runs in browser) and remote (connects to DefraDB node).
Tasks
How has this been tested?
Tested in browser with common queries like
addSchema,patchSchema,addView, andexecRequest.Specify the platform(s) on which this was tested: