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

chore: make templates bundle-able #593

Merged
merged 12 commits into from
Aug 6, 2024
Merged

Conversation

mingxuanzhangsfdx
Copy link
Member

@mingxuanzhangsfdx mingxuanzhangsfdx commented Jul 31, 2024

What does this PR do?

This PR removes the friction that blocks templates from bundling.

  1. transition the generator imports to static imports.
  2. remove the templates-bundle publishing workflow since templates will be bundle-able.

What issues does this PR fix or reference?

@W-15736930@


const sharedConfig = {
bundle: true,
format: 'cjs',
Copy link
Contributor

Choose a reason for hiding this comment

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

can the cli team consume a cjs module now that they are an esm shop? Might influence what we publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. I will have a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we do not need to bundle the project here. I think we do not need to take care of the issue.

templateType: TemplateType,
templateOptions: TOptions,
templateOptions: any,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this type changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Argument of type 'TOptions' is not assignable to parameter of type 'never'.
  The intersection 'AnalyticsTemplateOptions & ApexClassOptions & ApexTriggerOptions & LightningAppOptions & ... 7 more ... & VisualforcePageOptions' was reduced to 'never' because property 'template' has conflicting types in some constituents.
    Type 'TemplateOptions' is not assignable to type 'never'.ts(2345)

I ran into the issue above if I keep TOptions. I found that it will be much easier to just use any

package.json Outdated Show resolved Hide resolved

export async function importGenerator(templateType: TemplateType) {
const generatorClass =
TemplateType[templateType].toString().charAt(0).toLowerCase() +
TemplateType[templateType].toString().slice(1) +
'Generator';
return (await import(`../generators/${generatorClass}`)).default;
return generators.get(generatorClass) as Generators;
Copy link
Contributor

Choose a reason for hiding this comment

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

The as is masking the fact that generators.get(...) can return undefined. This flaw exists in the original code and assumes that the generator author has everything lined up properly. However, the original code would fail during the dynamic import if the calculated path to the template is incorrect.

How can the dev side of adding a template be made more concise and safe with this change in how we need to expose the generators for bundling?

templateType: TemplateType,
templateOptions: TOptions,
templateOptions: any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this type changed?

src/service/templateService.ts Outdated Show resolved Hide resolved
src/service/templateService.ts Outdated Show resolved Hide resolved
@mingxuanzhangsfdx mingxuanzhangsfdx marked this pull request as ready for review August 1, 2024 20:09
@mingxuanzhangsfdx mingxuanzhangsfdx changed the title chore: make templates bundle-able chore: make templates bundle-able Aug 1, 2024
@peternhale
Copy link
Contributor

QA Notes:

Linked this module to plugin-templates and smoke tested the template commands.

  • Commands that only have one template (project)
  • Commands that have multiple types (Apex class)
  • Used Default
  • Used named

Smoke tests worked fine.

@mingxuanzhangsfdx mingxuanzhangsfdx changed the title chore: make templates bundle-able chore: make templates bundle-able Aug 6, 2024
@mingxuanzhangsfdx mingxuanzhangsfdx merged commit 0a13978 into main Aug 6, 2024
8 checks passed
@mingxuanzhangsfdx mingxuanzhangsfdx deleted the mz/make-bundleable branch August 6, 2024 20:38
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.

3 participants