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

Use functions-core lib for Create Function command #3429

Closed

Conversation

YousefHaggyHeroku
Copy link
Contributor

@YousefHaggyHeroku YousefHaggyHeroku commented Jul 19, 2021

What does this PR do?

Replace CLI usage with library for Create Function, reducing execution time greatly. Additionally, add typescript support

What issues does this PR fix or reference?

@W-9620937@

Functionality Before

<insert gif and/or summary>

Functionality After

<insert gif and/or summary>

let showTextDocumentStub: SinonStub;
let gatherStub: SinonStub;
const functionInfoJS = {
type: 'CONTINUE' as 'CONTINUE',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hurts my eyes a little bit, but I get a typescript error when I don't include as CONTINUE

switch (language) {
case LANGUAGE_JAVASCRIPT:
metadata = MetadataDictionary.getInfo(FUNCTION_TYPE_JS);
metadata!.suffix = '.js';
Copy link
Contributor Author

@YousefHaggyHeroku YousefHaggyHeroku Jul 22, 2021

Choose a reason for hiding this comment

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

I don't think we need to use metadata!.suffix. A const fileExtension or something seems cleaner to me. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree! But, all the definitions in metadataDictionary (apexclass, triggers etc) all use suffix and this will be a much bigger refactor. I'm fine with that if you still want to, but can we make it a separate PR?

);
const document = await vscode.workspace.openTextDocument(outputFile);
vscode.window.showTextDocument(document);
channelService.appendLine('Installing dependencies...');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it's also worth using spawn instead of exec below and streaming the install output so users can see specifically what is going on

Copy link
Contributor

Choose a reason for hiding this comment

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

In this particular case, product wanted this to be a background process with limited user visibility and we don't show any notification popups either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I seem should I remove this line as well then?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on slack, this can remain as is.

@YousefHaggyHeroku YousefHaggyHeroku marked this pull request as ready for review July 22, 2021 18:53
@YousefHaggyHeroku YousefHaggyHeroku requested a review from a team as a code owner July 22, 2021 18:53
@@ -88,6 +88,7 @@ export type LocalComponent = DirFileNameSelection & {
suffix?: string;
};

export type FunctionInfo = DirFileNameSelection & {
export type FunctionInfo = {
fileName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we need the directory path anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so generateFunction takes language and figures out the path itself it looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think previously we used DirFileNameSelection which provided outputDir in order to satisfy the requirements of BaseTemplateCommand, but it was never actually set.

@YousefHaggyHeroku
Copy link
Contributor Author

Merged by #3476

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.

2 participants