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 frontend:dev:install configuration #1666

Merged
merged 4 commits into from
Aug 3, 2022
Merged

Add frontend:dev:install configuration #1666

merged 4 commits into from
Aug 3, 2022

Conversation

LGiki
Copy link
Contributor

@LGiki LGiki commented Jul 27, 2022

Add a frontend:dev:install configuration in wails.json to specify the dev dependencies installation command and update the docs.
Fixes #1663

@leaanthony leaanthony requested a review from stffabi July 30, 2022 03:31
@leaanthony leaanthony enabled auto-merge (squash) July 30, 2022 03:31
@leaanthony
Copy link
Member

This looks good to me! If @stffabi is happy, let's go!

@stffabi
Copy link
Collaborator

stffabi commented Jul 31, 2022

Thanks @LGiki for this. LGTM, there's just one thing which we might also need to change.

During the build process we also execute an install command before the frontend is getting build. Currently we use the frontend:install even when the build is started for a wails dev build. This feels somehow inconsistent when wails dev first would use frontend:dev:install (if it has been defined) but then the build process for the frontend would execute frontend:install.
I would propose to also use the same logic here if it's called for a dev build:

if b.projectData.InstallCommand == "" {
// No - don't install
outputLogger.Println(" - No Install command. Skipping.")
} else {

What do you think @leaanthony ?

@leaanthony
Copy link
Member

Yeah, good call. @LGiki this is what we do for the build command:

var buildCommand string
switch b.projectData.OutputType {
case "dev":
buildCommand = b.projectData.DevCommand
if buildCommand == "" {
buildCommand = b.projectData.BuildCommand
}
default:
buildCommand = b.projectData.BuildCommand
}
if buildCommand == "" {
outputLogger.Println(" - No Build command. Skipping.")
// No - ignore
return nil
}

@leaanthony
Copy link
Member

I'm thinking about how we can also change frontend:dev to frontend:dev:build for consistency. Maybe support both for a bit?

…f it has been defined) to install the dependencies
@LGiki
Copy link
Contributor Author

LGiki commented Aug 2, 2022

Thanks, I've pushed a new commit that uses the same logic to handle the install command when building the frontend if it's called for a dev build. And it might be better if I create a new PR to change frontend:dev to frontend:dev:build?

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

Looking good! Just one comment 👍👍👍

@@ -513,17 +513,27 @@ func (b *BaseBuilder) BuildFrontend(outputLogger *clilogger.CLILogger) error {
frontendDir := filepath.Join(b.projectData.Path, "frontend")

// Check there is an 'InstallCommand' provided in wails.json
if b.projectData.InstallCommand == "" {
var installCommand string
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified with the helper method you created! 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🚀 Thanks, I've simplified the logic of determining the installation command.

@leaanthony
Copy link
Member

Thanks, I've pushed a new commit that uses the same logic to handle the install command when building the frontend if it's called for a dev build. And it might be better if I create a new PR to change frontend:dev to frontend:dev:build?

That'd be great if you could! I would do something like we've done already and check frontend:dev:build but drop back to frontend:dev for compatibility reasons.

@LGiki
Copy link
Contributor Author

LGiki commented Aug 2, 2022

That'd be great if you could! I would do something like we've done already and check frontend:dev:build but drop back to frontend:dev for compatibility reasons.

Yeah, I will create a PR to do this. 🚀

Copy link
Collaborator

@stffabi stffabi left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , thanks so much for the time you have put into this.

@leaanthony leaanthony merged commit 5680159 into wailsapp:master Aug 3, 2022
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.

The frontend:dev configuration is used to both install dev dependencies and build the frontend
3 participants