search

grafana / lezer-logql

LogQL lezer grammar
Apache License 2.0
3 stars 4 forks source link

Remove lodash dependency #51

Closed matyax closed 1 year ago

matyax commented 1 year ago

While working on adding a new grammar I wanted to use the usual tree-viz and I noticed it was failing. I looked deeper and saw that it was failing because it couldn't find lodash.

error 1

error 2

IMO adding lodash as a dependency is not right, as it forces consumers of this library to have this dependency, and all that just for a trimEnd function.

In this PR I'm removing lodash and replacing it with a custom trimEnd implementation.

svennergr commented 1 year ago

I am not a big fan of reinventing the wheel. Obviously importing whole lodash just for trimEnd isn't great, but what about using https://www.npmjs.com/package/lodash.trimend?

matyax commented 1 year ago

I really don't think we need any dependency for 3 lines of code.

matyax commented 1 year ago

Additionally, we would have to either:

svennergr commented 1 year ago

I really don't think we need any dependency for 3 lines of code.

Next method we wanna use we say the same :D But also fine for me right now - nevertheless using dependencies shouldn't be a great show stopper.

Though, it's fine for me to not use any dependency for this.

matyax commented 1 year ago

We can definitely introduce dependencies in the near future if we need it. For now let's roll with this if you don't mind. Thanks!