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

Add SFDX: Create LWC Bundle command #277

Merged
merged 8 commits into from
Jan 30, 2018
Merged

Conversation

vazexqi
Copy link
Contributor

@vazexqi vazexqi commented Jan 27, 2018

What does this PR do?

  • Refactors the dependencies from salesforcedx-vscode-core to make it more shareable to other commands
  • Introduces the "SFDX: Create LWC Bundle" command to the command palette and right-click menu.

What issues does this PR fix or reference?

@W-4642992, W-4326859@

@@ -0,0 +1,41 @@
/*
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 moved all the command type declarations into salesforcedx-utils-vscode so we can share them.

@@ -10,6 +10,14 @@ import {
Command,
CommandExecution
} from '@salesforce/salesforcedx-utils-vscode/out/src/cli';
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, now we import them from the salesforcedx-utils-vscode package.

@@ -265,6 +273,21 @@ export async function activate(context: vscode.ExtensionContext) {
scratchOrgDecorator.showOrg();
scratchOrgDecorator.monitorConfigChanges();
}

const api: any = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We export these using https://code.visualstudio.com/docs/extensionAPI/vscode-api#_extensions

@DatGuyJonathan - I was deciding between the way we discussed and this way. Ultimately I felt this was better since it is more scalable. If we have other packages in the future, we want to allow them to add it in their package and not pollute core.

SfdxCommandlet,
SfdxCommandletExecutor,
SfdxWorkspaceChecker,
channelService,
Copy link
Contributor Author

@vazexqi vazexqi Jan 27, 2018

Choose a reason for hiding this comment

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

By exporting the singleton services, we ensure that there is only one instance of it so we won't have any weird behavior.

Had we somehow made what Gunnar was trying to do in microsoft/vscode-vsce#207 it still might not have worked because we lose the singleton-ness of the service and you could have multiple of the services activating.

import * as vscode from 'vscode';
import { nls } from '../messages';

const sfdxCoreExports = vscode.extensions.getExtension(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is how we import it.
The downside of this is that all of these things are exported as any so we lose the type safety... 😞

*
* If ommitted, we will assume _message.
*/
export const messages = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruthemmanuelle - These are the customer facing text. I followed what we had for lightning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, @vazexqi . I added one comment.

@vazexqi
Copy link
Contributor Author

vazexqi commented Jan 27, 2018

@Sweetman - Just a headsup since this affects #264 and the way we discussed.

@codecov
Copy link

codecov bot commented Jan 27, 2018

Codecov Report

Merging #277 into develop will increase coverage by 6.97%.
The diff coverage is 59.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #277      +/-   ##
===========================================
+ Coverage    75.07%   82.04%   +6.97%     
===========================================
  Files          116      134      +18     
  Lines         4414     5025     +611     
  Branches       728      833     +105     
===========================================
+ Hits          3314     4123     +809     
+ Misses         915      724     -191     
+ Partials       185      178       -7
Impacted Files Coverage Δ
...rcedx-vscode-core/src/commands/forceApexExecute.ts 89.58% <ø> (+89.58%) ⬆️
...forcedx-vscode-core/src/commands/forceOrgCreate.ts 79.16% <ø> (-11.31%) ⬇️
...core/src/commands/forceLightningComponentCreate.ts 100% <ø> (+100%) ⬆️
...vscode-core/src/commands/forceApexTriggerCreate.ts 42.85% <ø> (-5.53%) ⬇️
...re/src/commands/forceVisualforceComponentCreate.ts 100% <ø> (+100%) ⬆️
...de-core/src/commands/forceVisualforcePageCreate.ts 100% <ø> (+100%) ⬆️
...ode-core/src/commands/forceLightningEventCreate.ts 100% <ø> (+100%) ⬆️
...ges/salesforcedx-vscode-core/src/channels/index.ts 100% <ø> (ø) ⬆️
...ages/salesforcedx-vscode-core/src/messages/i18n.ts 100% <ø> (ø)
...cedx-vscode-core/src/commands/forceDebuggerStop.ts 77.27% <ø> (-14.62%) ⬇️
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab73f4...adb9858. Read the comment docs.

return vscode.Disposable.from(forceLightningLwcCreateCmd);
}

export async function activate(context: vscode.ExtensionContext) {
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 have to be careful while activating this command. It's actually possible for an extension to be installed without any of its extension-dependencies being installed (e.g, the user doesn't say install all dependencies, the user forcibly deletes the dependencies after, etc).

I need to have a check here for to ensure we don't activate if salesforcedx.salesforcedx-vscode-core exists and it has the api exported.

This is a bit more crucial because of the way the forceLightningLwcCreate gets its dependencies since the series of const on the top of that file.

*In the future, forceLightningLwcCreate is likely to move back into salesforcedx-vscode-core with the other create commands but for the time being, it's important to put it under here.

}
}

class ForceLightningLwcCreateExecutor extends (SfdxCommandletExecutor as {
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 syntax is because of microsoft/TypeScript#17032

const fileNameGatherer = new SelectFileName();
const lightningFilePathExistsChecker = new LightningFilePathExistsChecker();

export async function forceLightningLwcCreate(explorerDir?: any) {
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 cannot write a test (that will pass on our CI) for this until the next force-language-services@pre-release/release goes out since it has a dependency on that. The sfdx force:lightning:lwc:create command is in there. That is not going to happen until next week.

warning_prompt_lightning_bundle_overwrite:
'An LWC bundle with the specified path already exists in your workspace. Do you want to overwrite any existing files in this bundle?',
warning_prompt_yes: 'Yes',
warning_prompt_no: 'No'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change these from 'Yes' and 'No' to 'Overwrite' and 'Cancel'? We try to be as specific as possible about these confirmations, since people often just skim the text and don't necessarily realize what they're agreeing to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The text was taken from the other scaffolding commands. I'll make those changes over there as well. Specifically, I will create a newconfirm_overwrite_text

@@ -83,35 +83,43 @@
"explorer/context": [
{
"command": "sfdx.force.apex.class.create",
"when": "explorerResourceIsFolder && sfdx:project_opened"
"when":
"explorerResourceIsFolder && resourceFilename != aura && resourceFilename != lightningcomponents && sfdx:project_opened"
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 nice if we could create a re-usable compound when-clause. I don't think they have that support yet though.

@vazexqi vazexqi merged commit 5eb8a1e into develop Jan 30, 2018
@vazexqi vazexqi deleted the nick/add-lwc-create-command branch January 30, 2018 01:39
@sjurgis
Copy link

sjurgis commented Jun 30, 2018

What's a LWC bundle?

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.

5 participants