-
Notifications
You must be signed in to change notification settings - Fork 30
Fix breaking changes from v3.7.0 release #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
| /** Legacy export, maintained for backwards compatibility. | ||
| * See the comment for `legacyVectors`. | ||
| * @deprecated Use `vectors` instead. */ | ||
| export const vectorizer = legacyVectors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we revert this breaking change: vectorizer was an exported member, and even though we advertise its usage via configure.vectorizer, renaming it in this module is likely breaking.
| * | ||
| * */ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const __vectors_shaded = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously I tried doing some smart type hocus-pocus, but couldn't pull it off at the end: ModuleConfig part of ConfigureNonTextVectorizerOptions was loosing part of its generic type information.
I went with a simpler, albeit more verbose, solution. Keeping all pre-existing types intact, this simply overrides a selection of functions in vectors.
Add deprecation notice to none.
58a846f to
dabfd0e
Compare
ffd923f to
339dba7
Compare
| ? { | ||
| ...config, | ||
| // Only create modelId key if either config.model or config.modelId are defined. | ||
| ...(config?.modelId || config?.model ? { modelId: config?.model ?? config?.model } : undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax is fairly dense. My intention was to copy the model name into the modelId variable (for b/c) but preserve the previous behavior for config === undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I need to look a little more into the vectorizeCollectionName issue but we should be good to go.
Minor release on the branch?
977c2c3 to
98aefb1
Compare
This reverts commit 9f2a536.
ea53dd2 to
39c6572
Compare
We've made a number of breaking changes in the
v3.7.0release, which we're going to deprecate in favor ofv3.8.0.This PR brings back removed exports and deprecates
vectorizeCollectionNameparameter for all vectorizer modules where it has previously been accepted. See comment onlegacyVectorsfor more details.