Should we also add coverage for js.Object.Get and js.Object.Set methods? Are they equally affected?

No, they are not affected: https://gopherjs.github.io/playground/#/osNIk-Yqbn

Here's the relevant line in the compiler

This is entirely a nitpick, so ignore it if you wish... But I wonder if formatJSTag would be a better name, and more consistent with the other func names here, which use the <verb><object> convention (i.e. formatExpr and encodeIdent etc)

Done.

I feel like the word "js tag" is confusing. Isn't this actually a struct tag for js? How about formatStructTagForJS? (I'm not an English native speaker so my suggestion might not sound natural)

Addressed in the latest PR - I chose a slightly different version to keep more consistency with https://github.com/gopherjs/gopherjs/wiki/JavaScript-Tips-and-Gotchas

How about adding more test cases like starting with a char that is not for IDs?

I suspect we already have coverage for that elsewhere. It's good to avoid adding same tests in different places.

As there are such tests, the results should be changed?

I've made the test more "exciting" - I don't think any more are required because if we were doing the wrong thing then the generated Javascript would not parse.

Trivial style nitpick, maybe use Outer label. It'd be more consistent with:

(In general, RuneRange is not a label I see often, but Outer is pretty common.)

Done

Don't we need escaping some chars like backslash? text/template.JSEscapeString is needed here.

Ah that's a good catch; but we need strconv.Quote in this situation.

Also updated the test to include more special characters.

Fixed in latest commit.

Hmm, there are very subtle differences between strconv.Quote and template.JSEscapeString...

1. < and > are treated as special chars in JSEscapeString
2. strconv.Quote can produce \U000XXXXX for non-BMP and non-printable char, which is not valid escaping in ES5

You're absolutely right - thank you for correcting me!

I've just added a test in the latest commit that does a .Get in order to ensure the property that has been set via the js struct tag is using the expected key.

PTAL

JavaScript is spelled with a capital S, not "Javascript". See https://en.wikipedia.org/wiki/JavaScript, https://www.javascript.com/.

Also below.

Ok.

I think needsEscaping would be a more readable name.

That way it can start out with zero value:

var needsEscaping bool

And become set to true when encountering a character in the loop that needs escaping:

needsEscaping = true
break Outer

I'm not sure I agree. We want the default behaviour to be to use the dot notation because, as noted before, this is more efficient in output terms. "Dot notation" is taken directly from the MDN. To my mind it's better to have the variable name reflect the intended semantics, hence useDot.

We want the default behaviour to be to use the dot notation because, as noted before, this is more efficient in output terms.

My suggestion doesn't contradict that. needsEscaping controls whether non-default behavior of using escaping is used. If its value remains false, the default behavior (using dot notation) is used.

It's similar to when you handle error conditions (non-default behaviors), you typically write:

var err error // (controls whether non-default path will be used)
f, err = os.Open("file.txt")
if err != nil {
    // less common (non-default) path
    return fmt.Errorf("couldn't open file.txt: %v", err)
}
// normal (default) path

Instead of:

successfullyOpened := true
f, err := os.Open("file.txt")
if err != nil {
    successfullyOpened = false
}
if successfullyOpened {
    // normal (default) path
}
// less common (non-default) path
return fmt.Errorf("couldn't open file.txt: %v", err)

It's not a big deal, I just wanted to explain the rationale behind my suggestion.

My suggestion doesn't contradict that. needsEscaping controls whether non-default behavior of using escaping is used. If its value remains false, the default behavior (using dot notation) is used.

No I understand what you were getting at, it's just the name needsEscaping that doesn't make sense to me. I'm quite happy having the inverse logic, but if we were to do so then useBracket would probably be a better name (it's the alternative form).

I feel like this whole switch statement can be expressed more simply as:

ok := unicode.IsLetter(r) || (i != 0 && unicode.IsNumber(r)) || r == '$' || r == '_'
if !ok {
    needsEscaping = true
    break
}

Then no need for Outer: label either.

Inspired by https://github.com/gopherjs/snippet-store/blob/09ea5d200e03052ed3cc332e9c6e96ffbb6d6eb3/id.go#L37-L40.

Nice; yes that reads much better

I think we should indent the less frequent case, and keep the more common/normal case at normal indentation. So:

if needsEscaping {
    return "[\"" + template.JSEscapeString(jsTag) + "\"]"
}
return "." + jsTag

Again, per my comment above, I think useDot is clearer.

Why not just eliminate the variable entirely, and further shorten the function?

func formatJSStructTagVal(jsTag string) string {
    for i, r := range jsTag {
        ok := unicode.IsLetter(r) || (i != 0 && unicode.IsNumber(r)) || r == '$' || r == '_'
        if !ok {
            return "[\"" + template.JSEscapeString(jsTag) + "\"]"
        }
    }

    return "." + jsTag
}

To me this is clearer than either variable name alternative.

I think you meant to import html/template from the Go standard library rather than a new 3rd party package, right?

Oh I missed that... Thanks. I think text/template is enough (html/template is also fine and does the same thing).

goimports 😢 - thank you, good catch

Are you using the latest version of goimports? I think it's expected to prefer std lib packages over 3rd party ones when it matches both. If not, that's a bug and worth reporting.

I'll take a look, thanks for the tip.