wailsapp / wails

Create beautiful applications using Go
https://wails.io
MIT License
25.27k stars 1.22k forks source link

[v2] (wails:dev) smart rebuild #1031

Open ironpark opened 2 years ago

ironpark commented 2 years ago

Currently, the wails dev command rebuilds both the frontend and the backend even if the backend code changes that do not affect the frontend.

When the frontend's code base is light, it's not a big problem, but if the frontend build time exceeds a few minutes, it feels like a big problem in productivity.

How about conditionally skipping the frontend build by checking for changed files or checking if the generated "Wails JS" has changed?

leaanthony commented 2 years ago

Agreed! Happy to accept a PR for it!

leaanthony commented 2 years ago

PR to test: https://github.com/wailsapp/wails/pull/1071

ironpark commented 2 years ago

@leaanthony

In the last few days, after reviewing #1071 PR and looking at the source code related to the dev command, I found some areas for improvement.

leaanthony commented 2 years ago

Thanks for the feedback! I'll answer each point:

  • watch ignore It is hardcoded to omit the node_modules and build directories in the process of setting the directory to watch inside the initialiseWatcher function. It seems to be a better design to change this part so that it can be set as a flag.

Yeah, we could add this to the config.

  • add ignore .git directory It seems like a good idea to ignore the .git directory by default, like node_modules .

If we did the above config we could add this by default

  • recv watcher.Errors channel stream In our current implementation, we are only passing Events from watcher and not reading Errors . If messages accumulate in the Errors channel, watcher 's loop may hang and it may cause a potential bug that causes the build to stop.

Can you please provide more details on this - thanks!

  • go file ast check By using the default go/parser package, when the go file is changed, you can parse the file and check whether the build is possible by checking for errors. This prevents build from being triggered too often while editing the source code.

If you want to code this, I'd be happy to merge it. It's not a small task!

ironpark commented 2 years ago
for quit == false { // Line : 536
    select {
    case exitCode := <-exitCodeChannel:
                // ...
++  case err := <-watcher.Errors:
++      LogDarkYellow(err.Error())
    case item := <-watcher.Events:
        // ...
    case <-timer.C:
        // ...
    case <-quitChannel:
        // ...
    }
}
leaanthony commented 2 years ago

Regarding ignore dirs, we have 2 scenarios: "start with", eg build and "contains", eg "node_modules" because it could be far down the path. Do you think 2 flags are needed or is there a better way?

leaanthony commented 2 years ago

I'm ok pushing this out to v2.1 and seeing if we need it then.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ironpark commented 1 year ago

Regarding ignore dirs, we have 2 scenarios: "start with", eg build and "contains", eg "node_modules" because it could be far down the path. Do you think 2 flags are needed or is there a better way?

An approach like .gitignore is best in my opinion. saving in a file format such as .wailsignore or .watchignore is easy to modify.

It is convenient to be able to use wildcards to exclude directories as well as certain types of filenames.

For example, a test file like *_test.go excluding this can also reduce unnecessary builds.

leaanthony commented 1 year ago

Hey @Ironpark! Good to see you! I actually really like this idea, but I'm wondering if we can adopt the same idea but put it in wails.json to avoid multiple configs? Something like:

"dev:ignore": "frontend/my/dir/*.js, generated/*.go",

Thoughts?

ironpark commented 1 year ago

Good to see you too! :D Writing on a single line using , as a separator is prone to human error. I think writing in an array format like the one below reduces mistakes and is more readable.

"dev:ignore": [
    "frontend/my/dir/*.js",
    "generated/*.go",
    "*_test.go"
],
leaanthony commented 1 year ago

Of course 👍 Great idea.

ironpark commented 1 year ago

@leaanthony I came up with a more concrete approach to the AST-based file inspection I proposed last January, and wrote some POC code.

package main

import (
    "bytes"
    "fmt"
    "go/ast"
    "go/format"
    "go/parser"
    "go/token"
    "golang.org/x/tools/go/ast/astutil"
    "hash/fnv"
    "strings"
)

func fileHashWithAST(path string) (uint64, error) {
    fset := token.NewFileSet()
    node, err := parser.ParseFile(fset, path, nil, 0)
    if err != nil {
        return 0, err
    }
    badAst := false
    pre := func(c *astutil.Cursor) bool {
        switch c.Node().(type) {
        case *ast.ImportSpec:
            c.Delete()
        case *ast.BadExpr, *ast.BadDecl, *ast.BadStmt:
            badAst = true
            return false
        }
        return true
    }
    astutil.Apply(node, pre, nil)
    if badAst {
        return 0, fmt.Errorf("bad ast")
    }
    buf := bytes.Buffer{}
    if err := format.Node(&buf, fset, node); err != nil {
        return 0, err
    }
    cleanStr := strings.Replace(buf.String(), " ", "", -1)
    cleanStr = strings.Replace(cleanStr, "\t", "", -1)
    cleanStr = strings.Replace(cleanStr, "\r\n", "", -1)
    cleanStr = strings.Replace(cleanStr, "\n", "", -1)
    hash := fnv.New64a()
    _, _ = hash.Write([]byte(cleanStr))
    return hash.Sum64(), nil
}

func main() {
    fmt.Println(fileHashWithAST("main.go"))
}

The basic idea is this

  1. analyze the AST of the file
  2. remove import statements, comments
  3. remove all whitespace and newline characters and hash them.

This hash value is unchanged by comments, newlines, and import statements, so you can check for substantive logic changes.

When used in a file watcher

// Parsing failed: If the file is still being written, 
// a syntax error is assumed to have occurred due to an unfinished write.
if current_hash,err := ast_hash(file);err == nil{
    // Trigger a rebuild only if the hash value is different from before.
    if current_hash != before_hash[file] {
        before_hash[file] = current_hash
    // -> build update triggering
    }
}
leaanthony commented 1 year ago

That's quite impressive! Does it have any benefits over running the file through an md5 hasher? Incidentally, this is how task does file watching.

ironpark commented 1 year ago

That's quite impressive! Does it have any benefits over running the file through an md5 hasher? Incidentally, this is how task does file watching.

Fnv is a non-cryptographic hash function. It has no security requirements, so its implementation is simple and fast. I thought it was optimal for simply detecting changes to files as it is now.

Below is a microbenchmark. It's not very reliable due to the low number of iterations, but it's a good starting point.

BenchmarkFNV32-20        5614111           194.5 ns/op
BenchmarkFNV64a-20       6161049           198.0 ns/op
BenchmarkFNV128-20       5501001           201.5 ns/op
BenchmarkMD5-20          4314483           279.2 ns/op
BenchmarkSHA1-20         5616726           212.1 ns/op
BenchmarkSHA224-20       5642409           216.2 ns/op
BenchmarkSHA256-20       5274843           227.8 ns/op
BenchmarkSHA512-20       1779057           677.8 ns/op
ironpark commented 1 year ago

I took a moment to read the documentation. It looks like there are roughly 2-3 ways to use it with Task runner.

  1. Run the watch process in the wails cli as it is now and trigger the task script
    • This means that the watch function of the task is not used.
  2. Create a script or util that generates a checksum based on ast-hash and insert it into the execution condition of the task.
  3. Almost like the second method, but using '&&' to make the task ignore it when it runs
    • wails check {{TASK}} && go build ......
leaanthony commented 1 year ago

The reason I was asking is because we have a similar technique for monitoring package.json for updates. We run it through md5 and check it against package.md5

ironpark commented 1 year ago

Background

In fact, MD5 was invented as a cryptographic hash function. However, several security flaws have been found that can quickly discover hash collisions, so it is not recommended for security purposes.

However, it is still a hash function, so it's not bad for verifying file checksums, and its speed is not bad either.

But if you're considering a non-secure hash algorithm, FNV-1a is a good choice. It's fast with a sufficiently good hash distribution and collision resistance because it is implemented in the Go standard hash package.

If you don't care about standard packages, I'm not sure if there are properly implemented packages, but Murmur2/3 is also a good choice. It is faster for hashing large data due to structural advantages and has a slightly better hash distribution.

Personal Views

Given all of the above, if you're hashing a single package.json file, there's honestly no big deal to use MD5 or not - it's still useful in that use case and provides plenty of speed.

For more specific information on this topic, we recommend the following links which-hashing-algorithm-is-best-for-uniqueness-and-speed

leaanthony commented 1 year ago

I think we'll look for an existing library for rebuilding the app in v3. Vite already handles HMR for the frontend so we're only really dealing with the Go code. I imagine we'll get the rebuild trigger to run a task by default.