Skip to content

Conversation

himanshusinghs
Copy link
Contributor

Description

This is a follow up for the PR-5197 where we moved 'mongodb' to devDependencies in connection-form. In this PR we are restricting imports from 'mongodb' package to type only to avoid any possible regression in future.

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@himanshusinghs himanshusinghs added the no release notes Fix or feature not for release notes label Dec 7, 2023
@himanshusinghs
Copy link
Contributor Author

After checking the docs again, I figured that this won't work with inline type imports.
The other rule that I tried was no-restricted-syntax but that suffers from the same problem, does not work on inline type imports.

Now I could try writing a custom rule but I think we should probably get consistent with how we import types. What do you guys think?

@gribnoysup
Copy link
Collaborator

Can you clarify a bit what do you mean by inline type imports and where this would cause an issue?

@himanshusinghs
Copy link
Contributor Author

himanshusinghs commented Dec 7, 2023

By inline I meant this:

import { type X } from 'package-y';

We were earlier using the following syntax:

import type { X } from 'package-y';

but lately I saw a lot of usage of the inline type imports which sounds ok (maybe better in the way that they at-least allow import declaration of the package only once) but unfortunately that syntax fails for all the rules (no-restricted-syntax, no-extraneous-dependencies, no-restricted-imports) that I tried while attempting this restriction.

@gribnoysup
Copy link
Collaborator

Gotcha, thanks for context. IIRC this is mostly coming from eslint autofix and eslint autofixes to import { type <name> } only in cases where a mixed import is possible, so I wouldn't worry too much about it. The benefit of not accidentally pulling a non-type import for a package that we don't want to have in runtime seems to outweight the downside. Also this just seems like eslint plugin not being caught up with the new syntax, so it probably will eventually go away

@himanshusinghs
Copy link
Contributor Author

Yea that sounds about right, the plugin would eventually get updated. Alright I will keep it as is. Thank you :)

{
paths: [
{
name: 'mongodb',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think connection storage was another one that we only want as type-only dep here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I overlooked this one - added this in 247e5ca

@himanshusinghs himanshusinghs merged commit 32daaa8 into main Dec 8, 2023
@himanshusinghs himanshusinghs deleted the eslint-rule-disallow-mongodb-import branch December 8, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no release notes Fix or feature not for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants