Skip to content
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

Deployment change and autocomplete for Principal type #298

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

mmyslblocky
Copy link

We changed the method of deploying canisters via extension from sending the canister code in motoko to playground Deployer Canister and instead we added deployment through CLI by sending deployment commands to vscode terminal. We also added functionality to start local replica and deploy canisters to local node. The minor drawback of our solution is that we cannot automatically open Candid UI when canister deployment is finished because we VSCode Terminal API does not support reading output so we need to manually open Candid UI after deployment of canister is finished by selecting this option.

We've added auto complete for Principal primitive type and for shared function principal argument. There's suggestion for every available Principal method with short description.

@sa-github-api
Copy link

Dear @mmyslblocky,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA.

If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged.

— The DFINITY Foundation

Copy link
Contributor

@rvanasa rvanasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I added some feedback and will bring this up internally so the rest of the team can weigh in on these changes.

@@ -8,6 +8,8 @@
"runtimeExecutable": "${execPath}",
"args": ["--extensionDevelopmentPath=${workspaceFolder}"],
"outFiles": ["${workspaceFolder}/out/**/*.js"],
"stopOnEntry": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, I get a warning message saying "Property stopOnEntry is not allowed." Would it be possible to remove this, or is this important for development on your end?

image
Suggested change
"stopOnEntry": false,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sorry, I put this for test purpose of this extension only. I will remove it from PR

package.json Outdated
Comment on lines 137 to 149
"title": "Motoko: Start local replica..."
},
{
"command": "motoko.deployLocalSelection",
"title": "Motoko: Deploy (local node)..."
},
{
"command": "motoko.candidLocalSelection",
"title": "Motoko: Open Candid UI for canister (local node)..."
},
{
"command": "motoko.candidPlaygroundSelection",
"title": "Motoko: Open Candid UI for canister (playground)..."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, the ... suffix is used in this extension to indicate that you'll see a dropdown menu after selecting this command. Here are some suggested changes to the command titles:

Suggested change
"title": "Motoko: Start local replica..."
},
{
"command": "motoko.deployLocalSelection",
"title": "Motoko: Deploy (local node)..."
},
{
"command": "motoko.candidLocalSelection",
"title": "Motoko: Open Candid UI for canister (local node)..."
},
{
"command": "motoko.candidPlaygroundSelection",
"title": "Motoko: Open Candid UI for canister (playground)..."
"title": "Motoko: Start local replica"
},
{
"command": "motoko.deployLocalSelection",
"title": "Motoko: Deploy (local replica)..."
},
{
"command": "motoko.candidLocalSelection",
"title": "Motoko: Open Candid UI for canister (local replica)..."
},
{
"command": "motoko.candidPlaygroundSelection",
"title": "Motoko: Open Candid UI for canister (playground)..."

}),
);
context.subscriptions.push(
commands.registerCommand('motoko.startReplica', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also include a motoko.stopReplica command for completeness?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will include it

src/extension.ts Outdated
Comment on lines 127 to 130
dfx build ${canisterName ? canisterName : ''} &&
dfx deploy ${
canisterName ? canisterName : ''
} --playground`,
Copy link
Contributor

@rvanasa rvanasa Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability / conciseness:

Suggested change
dfx build ${canisterName ? canisterName : ''} &&
dfx deploy ${
canisterName ? canisterName : ''
} --playground`,
dfx build ${canisterName || ''} &&
dfx deploy ${canisterName || ''} --playground`,

src/extension.ts Outdated
placeHolder: 'Select canister to deploy',
});

if (selection === 'All') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth changing this logic to account for the possibility of a canister being named All, which is unlikely but possible.

@@ -0,0 +1,69 @@
import * as vscode from 'vscode';
import * as fs from 'fs';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would encourage using the fs/promises API to avoid latency during I/O operations.

@@ -0,0 +1,51 @@
import * as vscode from 'vscode';
import * as fs from 'fs';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above about using the fs/promises API.

@@ -1398,6 +1428,105 @@ connection.onDocumentSymbol((event) => {
return results;
});

function addPrincipalAutocomplete(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to eventually generalize this to all base library modules. I tested this and it seems to work as expected, so we could probably keep the hard-coded autocompletions for the time being.

@mmyslblocky
Copy link
Author

I think I adjusted the code to your review remarks, so you can verify these changes. I also signed the CLA agreement

@rvanasa
Copy link
Contributor

rvanasa commented Oct 17, 2024

Thank you for the updates. We are still deciding whether it makes sense to include these changes, since adding a dependency on dfx will break some of the functionality on native Windows.

The other question is whether we want to implement a more systematic autocompletion for all imports rather than just Principal. We are currently looking for someone who could do this as part of a developer grant in case you're interested in taking this a step further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants