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

[#121] Move Vite folder to packages #161

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

bterone
Copy link
Collaborator

@bterone bterone commented May 11, 2023

What happened πŸ‘€

βœ… Moved Vite template to under /packages directory
πŸ”΄ Removed lerna bootstrap command as it has been deprecated and now uses the package.json workspaces config πŸ˜„

βœ”οΈ Since moving the vite template to packages, the package-name %APP_NAME% is no longer valid. We've renamed to nimble-vite-template and we replace when generating the files

Insight πŸ“

πŸ™Œ With this PR, we allow for copying template files from the packages folder. This is only for the vite-template currently, and if there is NO template reference provided when running the generate command

For now we keep both Copying template files strategy and Downloading template strategy to keep options open in the future

πŸ”¨ Currently we're using @oclif/test which uses mocha and chai under the hood. We want to update to use jest so we can test all future changes following #116

Proof Of Work πŸ“Ή

Tests pass πŸŽ‰

@bterone bterone self-assigned this May 11, 2023
@bterone bterone force-pushed the feature/121-poc-move-vite-to-packages branch from 36f3d09 to f265315 Compare August 11, 2023 04:28
@bterone bterone force-pushed the feature/121-poc-move-vite-to-packages branch from 41db162 to 1e348f0 Compare August 11, 2023 05:19
@bterone bterone force-pushed the feature/121-poc-move-vite-to-packages branch from 7cfd708 to e4abfc6 Compare September 28, 2023 12:01
@bterone bterone force-pushed the feature/121-poc-move-vite-to-packages branch 2 times, most recently from fa93b0b to d2e6c4e Compare September 29, 2023 10:48
@bterone bterone force-pushed the feature/121-poc-move-vite-to-packages branch from d2e6c4e to ebbea7e Compare September 29, 2023 11:02
@bterone bterone changed the title POC - [#121] Move Vite folder to packages [#121] Move Vite folder to packages Oct 2, 2023
@bterone bterone marked this pull request as ready for review October 2, 2023 03:59
Copy link
Member

@malparty malparty left a comment

Choose a reason for hiding this comment

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

Small early suggestions πŸ˜‡

}

private async copyTemplateFiles(options: InitTemplateOptions): Promise<void> {
CliUx.ux.info('Copying template source files...');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CliUx.ux.info('Copying template source files...');
CliUx.ux.info('Copying ${selectedTemplate} template source files...');

Should/Can we add the template name in the log? πŸ‘€

Comment on lines 17 to 20
return this.downloadTemplateRepository(options)
.then(() => this.extractDownloadedTemplateFolder(options))
.then(() => this.renameFolder(options))
.then(() => this.cleanTemporaryFiles(options));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return this.downloadTemplateRepository(options)
.then(() => this.extractDownloadedTemplateFolder(options))
.then(() => this.renameFolder(options))
.then(() => this.cleanTemporaryFiles(options));
await this.downloadTemplateRepository(options);
await this.extractDownloadedTemplateFolder(options);
await this.renameFolder(options);
return await this.cleanTemporaryFiles(options);

Long time I haven't dealt with async JS, but as far as I remember, it acts the same but avoids the chained call (stack trace remains cleaner).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I wanted to keep changes to a minimum to make it easier to review, however I can change this πŸ‘ πŸ˜„

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 9f8df06 @malparty πŸ™Œ

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.

Move the /vite-template folder into the /packages folder
2 participants