search

google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.41k stars 1.15k forks source link

Implement OriginalMapping as an autovalue rather than protobuf message #4152

Closed niloc132 closed 8 months ago

niloc132 commented 9 months ago

Enables downstream projects to depend only on closure-compiler (and not protobuf-java) to read and write sourcemaps.

The proto file has shrunk over time, and only has a single message type, used to communicate details discovered when reading a sourcemap from JSON, composed into a message structure, and then back into separate values. The proto-based implementation doesn't serialize or deserialize, and doesn't appear to be involved in CompilerState even indirectly.

The replacement is slightly stricter - rather than marking each field as optional, only the identifier is optional.

brad4d commented 9 months ago

I'm importing this for testing and review now.

brad4d commented 9 months ago

It looks like closure-compiler would build fine with an AutoValue instead of a proto here, but I've discovered that several projects within Google depend on using the proto and expect it to be synchronized with closure-compiler's code.

I think we'll probably have to reject this PR, but I'm requesting further input from @concavelenz and other team members first to see if anyone has more information we should consider first.

brad4d commented 8 months ago

I spoke about this with @concavelenz

He said that although he'd like to accept this, it doesn't seem like a good idea given the other Google projects that depend on the protocol buffer definition.

niloc132 commented 8 months ago

Thanks for considering it - I do see that com.google.debugging.sourcemap.FilePosition has a TODO in its toString to rewrite as an autovalue. I have a separate commit that does that cleanup (10 files changed, 55 insertions, 71 deletions) which seems to have no sideeffects (in the closure-compiler codebase itself at least) - is that worth a PR?

No real benefit for me personally (aside from one fewer TODO, which is always nice), and I also don't want to make extra work for you and your team, but it does remove the TODO and makes that object immutable.

brad4d commented 8 months ago

Thanks for asking. I've just taken a look and found that FilePosition is also used by Google code that isn't part of closure-compiler, so you cannot see it to fix it in a PR. I doubt it's worth the work to change it. It's not actually wrong just less-than-ideal to have a "normal" class here instead of an @AutoValue.