-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(NODE-3245): mark symbols as internal remove from type definitions #2810
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
Looks like this caused a lint error:
|
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 like EventEmitterWithState
, RTTPinger
, RTTPingerOptions
are only used by classes we consider internal - so maybe we should mark these as internal as well?
@durran RTT* things have been made internal, the |
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.
LGTM. Nice work!
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.
LGTM 👍
marked as internal: - Topology - Connection - Server - Connection - ConnectionPool
d3a8199
to
176f7da
Compare
topology.once(Topology.OPEN, (error, topology) => | ||
mongoClient.emit(Topology.OPEN, error, topology) | ||
); | ||
topology.once(Topology.OPEN, () => mongoClient.emit('open', mongoClient)); |
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's the breaking change that is necessary to keep Topology @internal
and not exposed in the type definitions. This is arguably a more useful API as the topology object that was emitted here doesn't have any of the useful, db/collections APIs most desire to use.
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.
Need to figure out what we want to do with the remaining unused line in the built ts file; if we can't automatically fix the eslint@typescript-eslint/no-unused-vars rule, we can either 1) manually remove it via regex as an extra build step or 2) leave it be for now
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.
marked as internal:
I added a linter step to our outputted definitions file that removes the unused imports.
The event types had to be changed because there was a circular reference error. We are still type checking ourselves here though with the events, since we use, for example, the
Connection.COMMAND_STARTED
variable throughout the code, there would be type errors if this variable changes from the now hardcodedConnectionEvents
type.The Denque fix looks strange but the goal was to remove the Denque import from our type definitions and in order to do so we need to not use the
import Denque = require('denque')
syntax. Trying to enable es6 import syntax results in an error that requires you to enableesModuleInterop
but that uses a helper from tslib which we don't want to add as a dependency.NOTE: Most of the interesting changes are in the mongodb.d.ts file, mainly the removal of the large types listed above. It would be ideal to take a look there to see if there's anymore symbols that should be omitted from the public API.