search

Unleash / unleash-client-node

Unleash client SDK for Node.js
https://docs.getunleash.io
Apache License 2.0
212 stars 72 forks source link

chore: run lint:fix #593

Closed theneva closed 8 months ago

theneva commented 8 months ago

I ran yarn lint:fix on everything to make the code adhere to the Prettier rules. Ignoring renovate, there are no open pull requests at the moment, so I figured it's a good time to get this in.

Besides getting the code up to speed, this changeset provides a real benefit to me. We have to float our own patch of the client, so I maintain a fork with a single commit on top.

The current inconsistencies in the code tend to produce conflicts whenever I have to update our fork, which makes upgrading a bit of a pain. In practice, I have to manually format any new code I have to write for the patch, and be careful to never let my IDE try to help out.

In case you're interested, our patch (https://github.com/folio-as/unleash-client-node/commit/6e122a141f01647274e88c254d10430642be960e?diff=unified&w=1) prevents the client from appending every single OpenTelemetry span it produces to the same trace. Depending on its configuration, Tempo handles unleash-client's default behaviour in one of three ways:

  1. A lot of yelling about traces having too many spans
  2. A lot of yelling about traces having too many bytes
  3. It just blows up

This is practically the same issue that was addressed for the Ruby client in https://github.com/Unleash/unleash-client-ruby/pull/161. Given that https://github.com/Unleash/unleash-client-ruby/pull/150 was rejected, I expect that you don't want our patch upstreamed in the Node client either – but if I'm wrong, please let me know! I'd be more than happy to contribute it.

SimenB commented 8 months ago

Should probably add a yarn lint run as a step in https://github.com/Unleash/unleash-client-node/blob/main/.github/workflows/build-and-test.yaml to avoid regressing in the future

coveralls commented 8 months ago

Coverage Status

coverage: 90.863%. remained the same when pulling edb338c418c3a898f9f0618b77b762b17fd9b903 on theneva:main into 59b82c5b3c123588738fc32d4bec01d9b3aea58a on Unleash:main.

theneva commented 8 months ago

Should probably add a yarn lint run as a step in main/.github/workflows/build-and-test.yaml to avoid regressing in the future

I wholeheartedly agree. I think I'd like to keep this PR as is, though.

594 makes lint-staged run as a pre-commit again, which should also help to prevent further regression/drift.

thomasheartman commented 8 months ago

All the changes here appeared to have been merged with #594, so I'm closing this PR for now. Let me know if this is incorrect. And solid work! 👏🏼