This PR addresses the issue reported by @nicks, where a version.trim is not a function error is thrown under certain conditions.
The error occurs when a context value has a custom .toString() method that does not return a string. This was the only scenario in which we could reproduce the issue. A corresponding test has been added to validate this.
The solution suggested by @nicks has been implemented. Specifically, the !isStrictSemver(contextValue) check has been moved inside the try/catch block. This change ensures that any exceptions thrown by this method will be caught, in which case the function will default to false.
A more robust implementation of resolveContextValue has been introduced, which would also prevent the issue. See:
Lastly, we noticed the test files strategy-test.ts and flexible-rollout-test.ts were being ignored in test runs due to their filename format. Renaming them to strategy.test.ts and flexible-rollout.test.ts ensures they are included in the tests as intended.
coverage: 90.635% (+0.2%) from 90.456%
when pulling 28a584a0fab1508689b515f54975de08b58e07c8 on fix-context-edge-case-trim-not-a-function
into e61e55a3080b6ce12701831558d8121da68e3517 on main.
https://linear.app/unleash/issue/2-2468/fix-trim-is-not-a-function-edge-case-on-context-value
Fixes: https://github.com/Unleash/unleash-proxy/issues/185
This PR addresses the issue reported by @nicks, where a
version.trim is not a function
error is thrown under certain conditions.The error occurs when a context value has a custom
.toString()
method that does not return a string. This was the only scenario in which we could reproduce the issue. A corresponding test has been added to validate this.The solution suggested by @nicks has been implemented. Specifically, the
!isStrictSemver(contextValue)
check has been moved inside the try/catch block. This change ensures that any exceptions thrown by this method will be caught, in which case the function will default to false.A more robust implementation of
resolveContextValue
has been introduced, which would also prevent the issue. See:Lastly, we noticed the test files
strategy-test.ts
andflexible-rollout-test.ts
were being ignored in test runs due to their filename format. Renaming them tostrategy.test.ts
andflexible-rollout.test.ts
ensures they are included in the tests as intended.