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

Now able to generate LWCs using template files. #52

Merged
merged 13 commits into from
Nov 16, 2023

Conversation

dbreese
Copy link
Collaborator

@dbreese dbreese commented Nov 8, 2023

@dbreese dbreese requested a review from a team as a code owner November 8, 2023 22:39
@dbreese dbreese force-pushed the code_generate branch 2 times, most recently from eb3c983 to bc62e5e Compare November 9, 2023 13:37
@dbreese
Copy link
Collaborator Author

dbreese commented Nov 9, 2023

Ah, Windows is failing due to path separator issues. I will fix in update in a few minutes.

!quickActions.view
) {
// at least 1 needs to be created
const compactLayoutFields =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is expensive, so only doing it if we know we need to generate something.

@@ -165,17 +165,20 @@ suite('Org Utils Test Suite', () => {
{
editableForNew: true,
editableForUpdate: true,
label: 'Name'
label: 'Name',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could technically remove editableForNew/Update and this label value, because we thought we were going to use it and so far dont think we need to.

We still need to do some testing. It appears that the base LWC components used in our view/edit/create templates checks these values for us.

However, if we include "Owner", for example which has editableForNew=false and editableForUpdate=false, on the edit and create LWCs, it will still be shown, but will be disabled. Functionality still works fine. I think for the CREATE case, this may be odd because it will be a blank field shown, but still disabled. For the EDIT case, it still makes sense to show the value.

src/utils/codeBuilder.ts Outdated Show resolved Hide resolved
src/utils/codeBuilder.ts Outdated Show resolved Hide resolved
// ensure dirPath exists
if (!fs.existsSync(dirPath)) {
try {
fs.mkdirSync(dirPath, { recursive: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been a little IDK on this approach. Ideally we're operating from a place where the user has properly bootstrapped their project folder to be synced with the Offline Starter Kit project—where all of these folders should already exist. That it might not exist points to a more fundamental issue to me, than this code not functioning. But then you have to do some sanity checking beforehand, falling through on failure there, etc. ¯\(ツ)/¯ not sure of the right way to handle those boundary checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I.e. I'm not sure if we shouldn't be making more liberal use of the ConfigureProjectCommand.validateProjectFolder() method in the up-front validations of these commands that depend upon a functional project folder. Might be food for thought, for another day.

fieldNames.forEach((field) => {
var fieldNameImport = `${field.toUpperCase()}_FIELD`;
fields += `${fieldNameImport}, `;
imports += `import ${fieldNameImport} from "@salesforce/schema/${this.objectApiName}.${field}";\n`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it would be worthwhile to represent these template strings as constants at the top of the file? So that you'd have something like:

const IMPORT_STATEMENT_TEMPLATE = 'import <FieldNameImport> from "@salesforce/schema/<ObjectApiName>.<FieldName>";'
const LIGHTNING_INPUT_FIELD_TEMPLATE = '<lightning-input-field field-name={<FieldNameVariable>} value={<FieldName>}></lightning-input-field>';

And then do substitutions here? I can't decide if that would make things more readable or less readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure either. I am not a fan of all this string replacement, but not sure what else to do. Since it is hard to read already, moving the strings someplace else I almost feel like it could be more difficult to debug and/or maintain. But, I'm up for whatever!

Copy link
Collaborator

Choose a reason for hiding this comment

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

moving the strings someplace else I almost feel like it could be more difficult to debug and/or maintain

That was my concern too. I'm okay if we leave it, unless I think of some far more compelling way to make it more readable—which I haven't come up with yet. 😛

@dbreese
Copy link
Collaborator Author

dbreese commented Nov 15, 2023

Looks like it is failing for Lead LWC and QA generation. Debugging now. I also had to reboot vscode and clear node-modules and clean to finally get app to run again, so it could be related to that.

@dbreese
Copy link
Collaborator Author

dbreese commented Nov 15, 2023

Ahhhh, it is not finding Visit because I dont have the licenses set up in my org...crappy edge case that will probably bite a lot of people. Hmmm.....

templateVariables: TemplateVariables,
label: string,
name: string,
iconName?: string | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remaining nit: you don't need the | undefined. The ?: implies that it's either the specified type, or not provided—which equates to undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhhhh, sorry! Will update and push!

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

Looks good overall!

@dbreese dbreese merged commit 3f1a5ed into salesforce:main Nov 16, 2023
9 checks passed
@dbreese dbreese deleted the code_generate branch November 16, 2023 00:10
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

2 participants