AVRO-4350: [JavaScript] Restore underscore dependency - #3970
Conversation
RyanSkraba
left a comment
There was a problem hiding this comment.
Alright, this LGTM -- I don't want to abuse your expertise 😄 but can you advise us how to write a unit test (ideally run through ./build.sh test that would raise an error before this happens again?
I've put the smoke test in my release guidelines, but it would be great to catch these early!
|
Your thoughts on getting rid of the dependency are welcome -- if it's a risky or breaking change we can reserve it for the 1.13.0 release and keep it for the 1.12.x branch. |
Would you believe I did have a unit test for this originally and removed it! Haha. I have added it back.
I think it's relatively low risk and I will make sure that it doesn't have a breaking change when I do the PR. There are actually unit tests for this module but they don't run as part of the Mocha suite. I will port them to Mocha before I write any code and make sure my changes don't break them. |
|
Thanks so much! Cherry-picked to branch-1.12. Please don't hesitate to create other PRs, the javascript library appreciates all the love it can get! We can assist with Jira. |
|
Good stuff! I've raised the PR here: #3974. I also noticed that there are a bunch of over-rides in the |
What is the purpose of the change
AVRO-4350
Restore Underscore as a production dependency of
avro-js. The deprecated validator is loaded by the package entry point and still requires Underscore, causing clean installations to fail when requiringavro-js.Removing the Underscore usage from the deprecated validator is likely the better long-term solution. Since the currently published package is broken for clean installations, this PR prioritizes restoring the dependency as a small, low-risk fix. Removing the dependency can be handled separately.
Verifying this change
Added a test that loads the public package entry point. This reproduces the missing production dependency failure and runs as part of:
./build.sh testAlso manually verified by packaging the modified local source, installing the resulting tarball in a clean project, and successfully running:
node -e "require('avro-js')"The JavaScript lint task also passes.
Documentation