Skip to content
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

fix(NODE-3245): mark symbols as internal remove from type definitions #2810

Merged
merged 9 commits into from
May 21, 2021

Conversation

nbbeeken
Copy link
Contributor

marked as internal:

  • Topology
  • Connection
  • Server
  • Connection
  • ConnectionPool

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 hardcoded ConnectionEvents 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 enable esModuleInterop 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.

@nbbeeken nbbeeken requested review from a team, durran, emadum and dariakp and removed request for a team May 18, 2021 15:54
@emadum
Copy link
Contributor

emadum commented May 18, 2021

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 enable esModuleInterop but that uses a helper from tslib which we don't want to add as a dependency.

Looks like this caused a lint error:

Error: src/change_stream.ts:204:3 - (ae-forgotten-export) The symbol "DenqueLike" needs to be exported by the entry point index.d.ts

Copy link
Member

@durran durran left a 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?

@nbbeeken
Copy link
Contributor Author

@durran RTT* things have been made internal, the EventEmitterWithState cannot be made internal because it is depended upon by our topology and server events, however the stateChanged event can be made internal. Good suggestions!

@nbbeeken nbbeeken requested a review from durran May 19, 2021 14:16
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work!

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

src/change_stream.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
topology.once(Topology.OPEN, (error, topology) =>
mongoClient.emit(Topology.OPEN, error, topology)
);
topology.once(Topology.OPEN, () => mongoClient.emit('open', mongoClient));
Copy link
Contributor Author

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.

@nbbeeken nbbeeken requested a review from dariakp May 20, 2021 19:07
Copy link
Contributor

@dariakp dariakp left a 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

@nbbeeken nbbeeken requested a review from dariakp May 20, 2021 21:34
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@nbbeeken nbbeeken merged commit 0b636ba into 4.0 May 21, 2021
@nbbeeken nbbeeken deleted the NODE-3245/exports branch May 21, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants