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

feat(vscode): Introduce support for Static Web Apps #5099

Open
wants to merge 17 commits into
base: dev/StaticWebApps
Choose a base branch
from

Conversation

alain-zhiyanov
Copy link

@alain-zhiyanov alain-zhiyanov commented Jul 10, 2024

This adds support so users can create Static Web Apps alongside their Logic Apps when creating through 'Create New Logic App Workspace' command. I've changed how the user is prompted by the wizard, how files are created, and I've added calls to deploy the user's logic app after initialization and then call the Static Web Apps extension.

Some features of the original extension are not compatible at the moment.

@alain-zhiyanov alain-zhiyanov changed the title [DO NOT REVIEW] Adding support for Static Web Apps Adding support for Static Web Apps Jul 16, 2024
@alain-zhiyanov
Copy link
Author

@microsoft-github-policy-service agree [company="Microsoft"]

@alain-zhiyanov
Copy link
Author

@microsoft-github-policy-service agree

@alain-zhiyanov alain-zhiyanov marked this pull request as ready for review July 16, 2024 21:15
@alain-zhiyanov
Copy link
Author

@swghimire @ccastrotrejo

@ccastrotrejo ccastrotrejo changed the base branch from main to dev/StaticWebApps July 16, 2024 23:38
@ccastrotrejo
Copy link
Contributor

ccastrotrejo commented Jul 16, 2024

Updated the base branch to the feature branch dev/StaticWebApps as we try to keep everything on main branch ready to ship to prod @alain-zhiyanov

@ccastrotrejo ccastrotrejo changed the title Adding support for Static Web Apps feat(vscode): Introduce support for Static Web Apps Jul 16, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I think there is no need for an extra breakline here

Copy link
Author

Choose a reason for hiding this comment

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

automatically changed. let me know if I should revert

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please revert it

@@ -234,7 +234,6 @@
"7ZR1xr": "Add an action",
"7aJqIH": "Optional. The locale to be used when formatting (defaults to 'en-us').",
"7gUE8h": "This will revert your workflow to the state it was in before Copilot's edit. If you made additional edits to the workflow after Copilot's, you will lose them. This action cannot be undone. Do you want to continue?",
"7jAQar": "Enter value for parameter.",
Copy link

Choose a reason for hiding this comment

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

revert?

export async function generateStaticWebApp(context: IAzureConnectorsContext, node: vscode.Uri): Promise<void> {
console.log('generate SWA');
//removing "/c: because that doesn't fit SWA cli format"
const command: string = `node C:\\Users\\t-azhiyanov\\static-web-apps-cli-sifat\\dist\\cli\\bin.js generate ${node.path.substring(3)}`;
Copy link

Choose a reason for hiding this comment

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

how can this be a static path? Won't it fail to find the bits on another machine?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alain-zhiyanov I am not fully aware of the implementation or how we get this script, however if this is a static script, please upload the script in the repo or even better just make a function to call it. If its not static (generated), please make sure to have it a kind of static place for all users to work and also use path.join() since the pat diff for windows (\) is different than the one for linux (/)


export async function generateStaticWebApp(context: IAzureConnectorsContext, node: vscode.Uri): Promise<void> {
console.log('generate SWA');
//removing "/c: because that doesn't fit SWA cli format"
Copy link

Choose a reason for hiding this comment

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

Is this comment still needed?

import { ext } from '../../../extensionVariables';

export async function generateStaticWebApp(context: IAzureConnectorsContext, node: vscode.Uri): Promise<void> {
console.log('generate SWA');
Copy link

Choose a reason for hiding this comment

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

Since this is user facing can we make this more descriptive?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alain-zhiyanov Overall I would say to remove all console logs. If you want actually display something for the user to be aware of it, please the output channel and please localize the text. For example:
ext.outputChannel.appendLog(localize('excludedSettings', 'Excluded the following settings:'));

if (!projectPath) {
return;
}
//project path changed to account for SWA. need to add flag depending on if SWA initialized TODO
Copy link

@annikel annikel Jul 24, 2024

Choose a reason for hiding this comment

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

Is this backwards compatible yet? And why this the code commented

@@ -45,19 +46,68 @@ export class CodelessWorkflowCreateStep extends WorkflowCreateStepBase<IFunction
}

public async executeCore(context: IFunctionWizardContext): Promise<string> {
// Check if the user chose to initialize a static web app
if (context.initializeStaticWebApp) {
Copy link

Choose a reason for hiding this comment

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

Is the comment needed here?

Copy link

Choose a reason for hiding this comment

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

shouldInitializeStaticWebApp

let emptyStatefulDefinition: StandardApp;
if (context.initializeStaticWebApp) {
//initiate basic logic app which can be called from SWA
emptyStatefulDefinition = {
Copy link

Choose a reason for hiding this comment

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

should we read this from a file instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

we have a file to store the templates apps/vs-code-designer/src/app/utils/codeless/templates.ts

];

const placeHolder = localize('logicAppProjectTemplatePlaceHolder', 'Select a project template for your logic app workspace');
context.isCustomCodeLogicApp = (await context.ui.showQuickPick(picks, { placeHolder })).data;
const selectedPick = await context.ui.showQuickPick(picks, { placeHolder });

Copy link

Choose a reason for hiding this comment

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

nit: remove extra line

if (await confirmOverwriteFile(context, localSettingsJsonPath)) {
const localSettingsJson: ILocalSettingsJson = {
IsEncrypted: false,
Values: {
AzureWebJobsStorage: '',
APP_KIND: logicAppKind,
'AzureFunctionsJobHost__extensions__workflow__Settings__Runtime.Triggers.AnonymousAuthEnabledForDevEnvironment': 'true',
Copy link

Choose a reason for hiding this comment

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

nit: the formatting here is off

Copy link
Contributor

Choose a reason for hiding this comment

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

Also make sure to just add it when is needed for static web apps and not for all of the scenarios

@@ -43,12 +43,14 @@ export class ScriptProjectCreateStep extends ProjectCreateStepBase {
}

const localSettingsJsonPath: string = path.join(context.projectPath, localSettingsFileName);
//added new setting here to allow local start without SAS
Copy link

Choose a reason for hiding this comment

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

Are we always adding this setting now or only if is starting through swa cli?

await node.loadAllChildren(context);

//need to add flag for this call depending on if SWA initialized and remove region hardcode TODO
Copy link

Choose a reason for hiding this comment

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

Can we properly tag all todos and also add them to the doc

const logicAppBackendResourceId = siteConfig.id.substring(0, siteConfig.id.length - suffix.length);
vscodeExtension.commands.executeCommand('staticWebApps.createStaticWebApp', undefined, undefined, {
backendResourceId: logicAppBackendResourceId,
region: 'eastus',
Copy link

Choose a reason for hiding this comment

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

How can this region be static?

ILaunchJson,
IExtensionsJson,
} from '@microsoft/vscode-extension-logic-apps';
import { WorkflowProjectType, FuncVersion } from '@microsoft/vscode-extension-logic-apps';
import * as fse from 'fs-extra';
import * as path from 'path';
import type { TaskDefinition, DebugConfiguration, WorkspaceFolder } from 'vscode';
import { type TaskDefinition, type DebugConfiguration, type WorkspaceFolder, Uri } from 'vscode';
Copy link

Choose a reason for hiding this comment

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

nit: revert?

@@ -81,6 +77,9 @@ export abstract class InitVSCodeStepBase extends AzureWizardExecuteStep<IProject

context.telemetry.properties.isProjectInSubDir = String(isSubpath(context.workspacePath, context.projectPath));

//creating all .vscode files in the root of the workspace
Copy link

Choose a reason for hiding this comment

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

Can we work on adding the flags here?

@@ -114,53 +113,56 @@ export abstract class InitVSCodeStepBase extends AzureWizardExecuteStep<IProject
}

private async writeTasksJson(context: IProjectWizardContext, vscodePath: string): Promise<void> {
const newTasks: TaskDefinition[] = this.getTasks();
//writing custom Task json which will start LA and SWA. not following how tasks are originally added; should be fixed TODO
Copy link

Choose a reason for hiding this comment

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

Does this still have to be fixed?

const versionMismatchError: Error = new Error(
localize('versionMismatchError', 'The version in your {0} must be "{1}" to work with Azure Functions.', tasksFileName, tasksVersion)
);
const tasksJsonContent = `{
Copy link

Choose a reason for hiding this comment

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

can we read this from a file?

@@ -71,8 +69,12 @@ export async function addConnectionData(
}

export async function getLogicAppProjectRoot(context: IActionContext, workflowFilePath: string): Promise<string> {
const workspaceFolder = nonNullValue(getContainingWorkspace(workflowFilePath), 'workspaceFolder');
const workspacePath: string = workspaceFolder.uri.fsPath;
//need to add flag for how workspace found depending on if SWA initialized TODO
Copy link

Choose a reason for hiding this comment

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

Can we work on adding this flag

// return;
// }

const projectPath = path.join(searchDirectory(workspacePath, 'Stateful1'), '..');
Copy link
Contributor

Choose a reason for hiding this comment

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

is stateful1 always going to be the name of the workflow?

// Check if the user chose to initialize a static web app
if (context.initializeStaticWebApp) {
context.telemetry.properties.initializeStaticWebApp = 'true';
vscodeExtension.commands.executeCommand('azureLogicAppsStandard.deploy');
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Here is just better to call the function if possible, rather than calling the command

let emptyStatefulDefinition: StandardApp;
if (context.initializeStaticWebApp) {
//initiate basic logic app which can be called from SWA
emptyStatefulDefinition = {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a file to store the templates apps/vs-code-designer/src/app/utils/codeless/templates.ts

* @param {vscode.Task} task - Function task.
* @returns {number} Returns true if the task is a func host start task, otherwise returns false.
* @returns {boolean} Returns true if the task is an SWA start task, otherwise returns false.
*/
export function isFuncHostTask(task: vscode.Task): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this I would suggest creating a different function rather than just patching this one.

@@ -114,53 +113,56 @@ export abstract class InitVSCodeStepBase extends AzureWizardExecuteStep<IProject
}

private async writeTasksJson(context: IProjectWizardContext, vscodePath: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as for the func host start function. It would be great to have a different function

if (await confirmOverwriteFile(context, localSettingsJsonPath)) {
const localSettingsJson: ILocalSettingsJson = {
IsEncrypted: false,
Values: {
AzureWebJobsStorage: '',
APP_KIND: logicAppKind,
'AzureFunctionsJobHost__extensions__workflow__Settings__Runtime.Triggers.AnonymousAuthEnabledForDevEnvironment': 'true',
Copy link
Contributor

Choose a reason for hiding this comment

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

Also make sure to just add it when is needed for static web apps and not for all of the scenarios

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

Successfully merging this pull request may close these issues.

None yet

3 participants