-
Notifications
You must be signed in to change notification settings - Fork 53
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
Node: Fix node exports #2337
Node: Fix node exports #2337
Conversation
Signed-off-by: James Xin <[email protected]>
Duplicated naming found between |
Signed-off-by: James Xin <[email protected]>
node/npm/glide/index.ts
Outdated
ReturnTypeMap, | ||
ClusterResponse, | ||
ReturnTypeAttribute, | ||
PubSubSubscriptions, |
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.
Remove. PubSubSubscriptions is already exported as part of their respective namespaces.
createLeakedDouble, | ||
createLeakedMap, | ||
createLeakedString, | ||
parseInfoResponse, |
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.
@avifenesh can you confirm that these functions can be removed?
They were added as part of 5eeb673#diff-90759696d403370b4263a25799d215fe2cd1471e7ae440426c8177f636aa5bac, but it's unclear why they are needed (needed for alpine?)
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.
Note. These functions are only used in tests. We should consider reworking this. Maybe using a compile flag to generate these functions for test only. They do not need to be added to production code.
Also note: createLeakedStringVec
is used in production code, however this function is not exported from the package module.
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.
we cant remove it without fixing the release tests. For now, we can leave it as is . so I created a new PR to add the missing exports #2342
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
fixes: #2334