💅 useNodejsImportProtocol gets thrown although a npm package is installed with the same name

[corsin-ragettli]

Environment information

CLI:
  Version:                      1.8.0
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         unset
  JS_RUNTIME_VERSION:           "v18.18.2"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           true
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  false
  Recommended:                  false
  All:                          false
  Enabled rules:
  performance/noDelete
  suspicious/noCatchAssign
  suspicious/noUnsafeNegation
  complexity/useLiteralKeys
  style/useImportType
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessEmptyExport
  suspicious/useNamespaceKeyword
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  correctness/noConstantCondition
  a11y/useAriaActivedescendantWithTabindex
  suspicious/noAssignInExpressions
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noDuplicateParameters
  style/useTemplate
  correctness/noUnusedLabels
  complexity/noUselessTernary
  correctness/noUnreachableSuper
  suspicious/noCompareNegZero
  suspicious/noExplicitAny
  correctness/noSwitchDeclarations
  a11y/noAutofocus
  correctness/noUnsafeOptionalChaining
  correctness/noConstAssign
  suspicious/noControlCharactersInRegex
  complexity/noUselessTypeConstraint
  style/noVar
  suspicious/noDoubleEquals
  suspicious/noRedundantUseStrict
  style/useLiteralEnumMembers
  suspicious/noGlobalIsNan
  suspicious/noEmptyInterface
  suspicious/noConstEnum
  suspicious/noMisleadingCharacterClass
  correctness/noPrecisionLoss
  suspicious/noRedeclare
  correctness/noStringCaseMismatch
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  suspicious/noFallthroughSwitchClause
  suspicious/noUnsafeDeclarationMerging
  correctness/noUnreachable
  a11y/useKeyWithClickEvents
  suspicious/noDuplicateObjectKeys
  complexity/noUselessThisAlias
  complexity/noThisInStatic
  complexity/useOptionalChain
  correctness/noInnerDeclarations
  style/noParameterAssign
  suspicious/noDuplicateCase
  a11y/useValidAnchor
  complexity/useRegexLiterals
  correctness/noSelfAssign
  style/noUselessElse
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  a11y/useMediaCaption
  complexity/noUselessLabel
  complexity/noUselessCatch
  correctness/noUnsafeFinally
  a11y/useAriaPropsForRole
  correctness/noNonoctalDecimalEscape
  style/useEnumInitializers
  a11y/useHtmlLang
  suspicious/noDuplicateTestHooks
  complexity/noStaticOnlyClass
  style/useWhile
  complexity/useArrowFunction
  style/noInferrableTypes
  a11y/noNoninteractiveTabindex
  complexity/useSimpleNumberKeys
  correctness/useYield
  a11y/noInteractiveElementToNoninteractiveRole
  style/useNumericLiterals
  correctness/noUnnecessaryContinue
  suspicious/noApproximativeNumericConstant
  suspicious/noImportAssign
  suspicious/noLabelVar
  correctness/noGlobalObjectCalls
  suspicious/useDefaultSwitchClauseLast
  a11y/useAltText
  correctness/noEmptyCharacterClassInRegex
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noSparseArray
  a11y/useIframeTitle
  complexity/noBannedTypes
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  style/useAsConstAssertion
  correctness/useJsxKeyInIterable
  style/useExportType
  complexity/noUselessLoneBlockStatements
  suspicious/noPrototypeBuiltins
  suspicious/noMisleadingInstantiator
  suspicious/noDebugger
  style/noArguments
  a11y/useValidAriaValues
  suspicious/noCommentText
  suspicious/noThenProperty
  suspicious/noDuplicateJsxProps
  suspicious/noGlobalAssign
  a11y/noPositiveTabindex
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  security/noDangerouslySetInnerHtmlWithChildren
  a11y/useKeyWithMouseEvents
  suspicious/noExtraNonNullAssertion
  suspicious/useGetterReturn
  correctness/noRenderReturnValue
  correctness/useExhaustiveDependencies
  security/noGlobalEval
  style/noNonNullAssertion
  a11y/noRedundantRoles
  complexity/useFlatMap
  correctness/useIsNan
  style/useConst
  suspicious/noGlobalIsFinite
  suspicious/noSelfCompare
  suspicious/noAsyncPromiseExecutor
  security/noDangerouslySetInnerHtml
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  suspicious/noArrayIndexKey
  complexity/noWith
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  correctness/noChildrenProp
  suspicious/noConfusingLabels
  suspicious/noConfusingVoidType
  suspicious/noFocusedTests
  a11y/useButtonType
  a11y/noAriaUnsupportedElements
  correctness/noFlatMapIdentity
  a11y/noBlankTarget
  a11y/useHeadingContent
  correctness/useValidForDirection
  correctness/noVoidTypeReturn
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useAnchorContent
  complexity/noUselessRename
  correctness/noInvalidNewBuiltin
  style/useNumberNamespace
  complexity/noUselessConstructor
  a11y/noAccessKey
  style/useExponentiationOperator
  style/noUnusedTemplateLiteral
  complexity/noUselessSwitchCase
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  suspicious/useIsArray
  a11y/noHeaderScope
  complexity/noUselessFragments
  suspicious/noMisrefactoredShorthandAssign
  complexity/noForEach
  suspicious/noClassAssign
  suspicious/noFunctionAssign

Workspace:
  Open Documents:               0

Rule name

useNodejsImportProtocol

Playground link

https://github.com/corsin-ragettli/biome-nodeProtocol-error

Expected result

When a npm package with the same name as a node module is installed, don't throw a useNodejsImportProtocol error

Code of Conduct

• I agree to follow Biome's Code of Conduct

[vctqs1]

@corsin-ragettli Could you share the package name you would like install?

[corsin-ragettli]

Hi, @vctqs1, Im having issues with the querystring package. I know it's deprecated but I use it in an old project...

[Conaclos]

We could use the Manifest query to not apply the rules on project dependencies. Unfortunately if package.json is not present, the rule will not apply. We should introduce a way of querying the manifest without requiring it.

[ematipico]

We could use the Manifest query to not apply the rules on project dependencies. Unfortunately if package.json is not present, the rule will not apply. We should introduce a way of querying the manifest without requiring it.

I think it should be easy enough. We can create a get_service method inside the RuleContext type

[corsin-ragettli]

We could use the Manifest query to not apply the rules on project dependencies. Unfortunately if package.json is not present, the rule will not apply. We should introduce a way of querying the manifest without requiring it.

In this case, a package.json should always be present since we installed the dependency, right? Or am I missing something here?

[Conaclos]

In this case, a package.json should always be present since we installed the dependency, right? Or am I missing something here?

You could write a file executable by node without any package.json. Also, I am not sure if we currently resolve package.json if Biome is executed from an inner folder.

[ematipico]

We could use the Manifest query to not apply the rules on project dependencies. Unfortunately if package.json is not present, the rule will not apply. We should introduce a way of querying the manifest without requiring it.

In this case, a package.json should always be present since we installed the dependency, right? Or am I missing something here?

No, that's an assumption we can't do. Biome can be run anywhere. It can be installed globally because it's a binary, and it can analyse files or projects that aren't inside a package.json. Users can also install the binary inside editors and use a bundled binary.

[corsin-ragettli]

We could use the Manifest query to not apply the rules on project dependencies. Unfortunately if package.json is not present, the rule will not apply. We should introduce a way of querying the manifest without requiring it.

In this case, a package.json should always be present since we installed the dependency, right? Or am I missing something here?

No, that's an assumption we can't do. Biome can be run anywhere. It can be installed globally because it's a binary, and it can analyse files or projects that aren't inside a package.json. Users can also install the binary inside editors and use a bundled binary.

Fair enough, thanks for the explanation.

I would like to start contributing to this project, but it sounds like this is the wrong issue to do that. What do you guys think?

[Sec-ant]

Also, I am not sure if we currently resolve package.json if Biome is executed from an inner folder.

We do, we use the same auto_search function used by biome configuration resolution to search for package.json files.

But when Biome is executed from the project root against a file in an inner directory, it will start searching from the root instead of that inner directory for manifest files. This is a known bug that will affect some monorepo projects.

[lishaduck]

May I ask why you're doing this, @corsin-ragettli?
Polyfilling for old Node.JS versions doesn't make sense as node:querystring has been around since node 0.1.25.
Polyfilling for the web doesn't make sense as require() isn't supported there?
Preferring pure JS doesn't make sense, as require('querystring') will map to require('node:querystring'), not require('querystring/') (mysticatea/eslint-plugin-node#69).

I would think that useNodejsImportProtocol is still applicable, honestly more so, as the new behavior masks the above require behavior. I also just tested, and the same behavior is found with import, although the / workaround doesn't work, as the import module resolver doesn't support importing index.js for directories.

(I can set up a gist with a reproduction for the import behavior, if desired)

_{^{Presumably, this should be a new issue, but 🤷‍♂️}}

[corsin-ragettli]

May I ask why you're doing this, @corsin-ragettli? Polyfilling for old Node.JS versions doesn't make sense as node:querystring has been around since node 0.1.25. Polyfilling for the web doesn't make sense as require() isn't supported there? Preferring pure JS doesn't make sense, as require('querystring') will map to require('node:querystring'), not require('querystring/') (mysticatea/eslint-plugin-node#69).

I would think that useNodejsImportProtocol is still applicable, honestly more so, as the new behavior masks the above require behavior. I also just tested, and the same behavior is found with import, although the / workaround doesn't work, as the import module resolver doesn't support importing index.js for directories.

(I can set up a gist with a reproduction for the import behavior, if desired)

Presumably, this should be a new issue, but 🤷‍♂️

Hi, sorry for the late reply. I found this behavior in a super old repo where I haven't been working on a lot, so I don't know the in's and out's of this code. In the codebase we're actually using it in the web (via import syntax). I didn't want to look into weird behavior differences between the node implementation and the npm version, so I left it at that.

I would imagine the import for the web uses the npm package, since you can't import node modules in the web (that's just an assumption tough).

[lishaduck]

In the codebase we're actually using it in the web (via import syntax).

Ah, checked that repro again, and it is import. Not sure why I thought you were using require.

I would imagine the import for the web uses the npm package, since you can't import node modules in the web (that's just an assumption tough).

I would think so as well, but, when I think about it, importing bare specifiers isn't valid either. I assume you're bundling then, which makes me wonder if you're polyfilling in your build process.

Anyway, I think that makes sense. Thanks for explaining. 😁