remove hard dependency on protobufjs #19
Conversation
rochdev
force-pushed
the
rochdev/remove-protobuf-dependency
branch
from
43fefb3 to
f9f6e2d
Compare
|
Discussed offline with @brimtown and for now the dependency will simply be removed and become optional. The API can be revisited in a future version if needed to support additional serialization formats. This is now ready for review. |
| } | ||
|
|
||
| toProto(): IStore { | ||
| const ProtoStore = require('../proto/compiled').Store; |
There was a problem hiding this comment.
Just out of curiosity, did you try to get these to work with dynamic import? Benefit would be not needing the lint override.
There was a problem hiding this comment.
This would change the API since it makes the call asynchronous and uses a promise.
README.md
Outdated
| ```sh | ||
| # NPM | ||
| npm install @datadog/sketches-js | ||
| npm install --save @datadog/sketches-js |
There was a problem hiding this comment.
Do we need this? Wasn't familiar with it, but seems like this has been the default behavior for a while (source).
There was a problem hiding this comment.
I didn't use npm for years and wasn't aware of the change, good catch!
| } | ||
|
|
||
| toProto(): IIndexMapping { | ||
| const ProtoIndexMapping = require('../proto/compiled').IndexMapping; |
There was a problem hiding this comment.
There are still some static imports to /proto/compiled at the top of this file, does that matter? I would sort of expect that any one of those would cause the module missing error, regardless of if an end user is invoking toProto / fromProto.
There was a problem hiding this comment.
It seems that the TypeScript compiler is able to understand that actual code isn't used and not add the require call.
This PR removes the hard dependency on Protobuf. The library can be used just to capture metrics as histograms without serialization, in which case this large dependency is pulled unnecessarily. If Protobuf is needed, then having an explicit dependency on it by the app instead will still make it available, and otherwise calling the methods that need it will error out.
This is a breaking change for any app that is not explicitly depending on
protobufjsso a 2.0 release would be needed.A few open questions about the implementation:
protobufjsis missing instead of just the standard missing module error?fromProtoandtoProtomethods?