Remove detection of files via import resolution, use static glob and npmignore #788

jakebailey · 2023-10-27T00:29:17Z

Fixes #769

Files are now determined statically by extension only, ignoring versioned subdirs.

For one-off npm pack in the DT repo, an .npmignore is now required. For react, this looks like:

*
!**/*.d.ts
!**/*.d.cts
!**/*.d.mts
!**/*.d.*.ts
/v15/
/v16/
/v17/

As outlined in #769, this is the "least bad" solution as far as I can tell, other than to just allow any file in random npm packs and ignore the subdirs.

This required a bunch of lint changes, including moving the relative import check to a lint rule, which required a bunch of other changes.

This is still a WIP with a bunch of TODOs, but it's good enough to start looking at.

TODO

Give feedback

Make having OTHER_FILES.txt an error
Further test new lint rule
Switch more stuff to isDeclarationPath
Make new lint rule not use module resolution
Dedupe new lint rule with no-bad-reference
new dtslint tests for new thrown errors
Determine proper signal for "is dt root" and "is types package"
Options

jakebailey · 2023-10-27T16:30:16Z

This PR still uses the same module resolution code to check errors, but that actually nets the same bugs that we are seeing with files today, just in lint form:

Error in node-notifier/v5
Error: 
/home/jabaile/work/DefinitelyTyped/types/node-notifier/v5/index.d.ts
  5:42  error  The relative import "node-notifier/notifiers/notificationcenter" resolves outside of the package; use a bare import to reference other packages  @definitelytyped/no-relative-references
  6:34  error  The relative import "node-notifier/notifiers/notifysend" resolves outside of the package; use a bare import to reference other packages          @definitelytyped/no-relative-references
  7:38  error  The relative import "node-notifier/notifiers/toaster" resolves outside of the package; use a bare import to reference other packages             @definitelytyped/no-relative-references
  8:38  error  The relative import "node-notifier/notifiers/balloon" resolves outside of the package; use a bare import to reference other packages             @definitelytyped/no-relative-references
  9:29  error  The relative import "node-notifier/notifiers/growl" resolves outside of the package; use a bare import to reference other packages               @definitelytyped/no-relative-references

/home/jabaile/work/DefinitelyTyped/types/node-notifier/v5/node-notifier-tests.ts
  28:38  error  The relative import "node-notifier/notifiers/notificationcenter" resolves outside of the package; use a bare import to reference other packages  @definitelytyped/no-relative-references
  31:30  error  The relative import "node-notifier/notifiers/notifysend" resolves outside of the package; use a bare import to reference other packages          @definitelytyped/no-relative-references
  34:34  error  The relative import "node-notifier/notifiers/toaster" resolves outside of the package; use a bare import to reference other packages             @definitelytyped/no-relative-references
  37:25  error  The relative import "node-notifier/notifiers/growl" resolves outside of the package; use a bare import to reference other packages               @definitelytyped/no-relative-references
  40:34  error  The relative import "node-notifier/notifiers/balloon" resolves outside of the package; use a bare import to reference other packages             @definitelytyped/no-relative-references

✖ 10 problems (10 errors, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:172:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:150:13)


Error in parse/v1
Error: 
/home/jabaile/work/DefinitelyTyped/types/parse/v1/index.d.ts
  1149:28  error  The relative import "parse/node" resolves outside of the package; use a bare import to reference other packages  @definitelytyped/no-relative-references
  1156:28  error  The relative import "parse/node" resolves outside of the package; use a bare import to reference other packages  @definitelytyped/no-relative-references

✖ 2 problems (2 errors, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:172:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:150:13)

This fails because module resolution doesn't see declare module on a self package, which we generally ban but people have disabled (boo). As such, these are versioned packages so they do resolve to the parent package!

I'm going to update the PR to instead not use module resolution, as everything we need can actually be done directly on the paths.

packages/eslint-plugin/src/util.ts

sandersn

Some comments, although I still need to review the new eslint-plugin changes.

.vscode/settings.json

packages/utils/src/miscellany.ts

packages/dtslint/src/checks.ts

.knip.jsonc

packages/utils/src/fs.ts

jakebailey · 2023-10-31T23:25:27Z

Latest run caught some very fun stuff:

Error in gauge
Error: 
/home/jabaile/work/DefinitelyTyped/types/gauge/lib/themes/index.d.ts
  1:24  error  The reference "../../../node" resolves outside of the package. Use a global reference to reference other packages  @definitelytyped/no-bad-reference

✖ 1 problem (1 error, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)


Error in rangy
Error: 
/home/jabaile/work/DefinitelyTyped/types/rangy/rangy-tests.ts
  1:24  error  The reference "../rangy/rangy-classapplier" resolves to the current package, but uses relative paths  @definitelytyped/no-bad-reference

✖ 1 problem (1 error, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)


Error in slickgrid
Error: 
/home/jabaile/work/DefinitelyTyped/types/slickgrid/test/slick.autotooltips.ts
  1:24  error  The reference "../../slickgrid/slick.autotooltips" resolves to the current package, but uses relative paths  @definitelytyped/no-bad-reference

/home/jabaile/work/DefinitelyTyped/types/slickgrid/test/slick.checkboxselectcolumn.ts
  1:24  error  The reference "../../slickgrid/slick.checkboxselectcolumn" resolves to the current package, but uses relative paths  @definitelytyped/no-bad-reference
  2:24  error  The reference "../../slickgrid/slick.rowselectionmodel" resolves to the current package, but uses relative paths     @definitelytyped/no-bad-reference
  3:24  error  The reference "../../slickgrid/slick.columnpicker" resolves to the current package, but uses relative paths          @definitelytyped/no-bad-reference

✖ 4 problems (4 errors, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)


Error in strophe.js
Error: 
/home/jabaile/work/DefinitelyTyped/types/strophe.js/strophe.js-tests.ts
  1:24  error  The reference "../strophe.js/muc" resolves to the current package, but uses relative paths  @definitelytyped/no-bad-reference

✖ 1 problem (1 error, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)


Error in waypoints
Error: 
/home/jabaile/work/DefinitelyTyped/types/waypoints/waypoints-tests.ts
  1:24  error  The reference "../waypoints/jquery" resolves to the current package, but uses relative paths  @definitelytyped/no-bad-reference

✖ 1 problem (1 error, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)


Error in react-is
Error: 
/home/jabaile/work/DefinitelyTyped/types/react-is/test/canary-tests.tsx
  1:24  error  The reference "../../react/experimental" resolves outside of the package. Use a global reference to reference other packages  @definitelytyped/no-bad-reference

✖ 1 problem (1 error, 0 warnings)

    at testTypesVersion (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:173:15)
    at async runTests (/home/jabaile/work/DefinitelyTyped-tools/packages/dtslint/dist/index.js:151:13)

sandersn

I have to run to the dentist but I'm all done reviewing except for reading the new test infrastructure and assertions.

packages/eslint-plugin/src/util.ts

packages/eslint-plugin/src/rules/no-bad-reference.ts

packages/eslint-plugin/test/fixtures/types/no-bad-reference/v0.1/index.d.ts

packages/eslint-plugin/test/fixtures/types/no-bad-reference/v11/index.d.ts

packages/eslint-plugin/test/fixtures/types/no-relative-references/v1/index.d.ts

packages/eslint-plugin/test/fixtures/types/no-self-import/index.d.ts

packages/eslint-plugin/test/no-bad-reference.test.ts

sandersn · 2023-11-01T21:43:16Z

packages/eslint-plugin/test/no-declare-current-package.test.ts

-        },
-      ],
+      filename: "types/no-declare-current-package/index.d.ts",
+      errors: [{ messageId: "noDeclareCurrentPackage" }],


this is missing data, which made the tests easier to read in no-bad-reference. Is it possible to get data here too?

Yes, I just didn't add it since I copied the original test. Will fix.

Done; that caught the fact that the lint rule forgot a quote.

sandersn

Finished up looking at tests.

jakebailey added 28 commits October 26, 2023 16:04

Some progress

82431ea

Make prettier happier

4b69bc5

I'm sick of ts-jest warnings

fad3203

Shifting, but it broke

c48ad17

Drop todo, async hard

8f996b8

Add rule

2d45701

Starting some testing

09fd107

maybe fix pos info

47a9344

Disable error reporting for now, files array is no good

05b3379

Off by one

b56bd29

It works

79e5389

More temp removal

e2bf963

better messaging

b84884d

Move no-import-of-dev-dependencies to fixtures

5cdb31f

Silence jest

2b99dab

Move no-self-import to new fixtures

beb5ec2

Fix name of test

9ce00dd

Move no-declare-current-package to new fixtures

e952bca

Test utils

77590a3

Improvements

8f02788

Move to npmignore

050ab17

TODO

a4384ca

missing change

b8d7493

tweak helper

df63da3

Remove old test

dd4a607

Unused import

a22273b

Remove old test

2a957b6

Resolve TODOs

5e2bd4a

jakebailey commented Oct 27, 2023

View reviewed changes

packages/eslint-plugin/src/util.ts Outdated Show resolved Hide resolved

Merge branch 'master' into npmignore

b84865e

sandersn reviewed Oct 31, 2023

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

packages/utils/src/miscellany.ts Outdated Show resolved Hide resolved

packages/dtslint/src/checks.ts Outdated Show resolved Hide resolved

Undo leftover change

7b1d440

jakebailey commented Oct 31, 2023

View reviewed changes

.knip.jsonc Outdated Show resolved Hide resolved

jakebailey added 3 commits October 31, 2023 14:30

Consolidate logic for types package prefix

f945014

One more use

29bce41

Rename

aa23f5e

jakebailey commented Oct 31, 2023

View reviewed changes

packages/utils/src/fs.ts Outdated Show resolved Hide resolved

jakebailey mentioned this pull request Oct 31, 2023

Fix bad references DefinitelyTyped/DefinitelyTyped#67264

Merged

Merge branch 'master' into npmignore

656b019

sandersn reviewed Nov 1, 2023

View reviewed changes

jakebailey added 4 commits November 1, 2023 10:03

Inline constant (it used to be more complicated)

6f45d35

Remove unused comment

a869d6e

Move Reference up

435921f

PR feedback

e2aba74

sandersn requested changes Nov 1, 2023

View reviewed changes

jakebailey added 2 commits November 1, 2023 15:20

Merge branch 'master' into npmignore

7e2a8ea

Fix no-declare-current-package and improve tests

4b3f361

sandersn approved these changes Nov 1, 2023

View reviewed changes

This was referenced Nov 2, 2023

Self references are local #103

Closed

[definitions-parser] skip scripts folder #812

Merged

Ignore .npmignore changes; checked in CI DefinitelyTyped/dt-mergebot#483

Merged

jakebailey added 3 commits November 6, 2023 10:32

Merge branch 'master' into npmignore

ee4f887

Update content hash to sort package.json, no BOM check

817d149

Ban BOM via eslint

0f45bff

jakebailey merged commit a18ce6b into microsoft:master Nov 6, 2023
6 checks passed

jakebailey deleted the npmignore branch November 6, 2023 21:39

jakebailey mentioned this pull request Nov 6, 2023

Generate .npmignore, set version X.Y.9999 microsoft/dts-gen#189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove detection of files via import resolution, use static glob and npmignore #788

Remove detection of files via import resolution, use static glob and npmignore #788

jakebailey commented Oct 27, 2023 •

edited

Loading

TODO

jakebailey commented Oct 27, 2023 •

edited

Loading

sandersn left a comment

jakebailey commented Oct 31, 2023

sandersn left a comment

sandersn Nov 1, 2023

jakebailey Nov 1, 2023

jakebailey Nov 1, 2023

sandersn left a comment

Remove detection of files via import resolution, use static glob and npmignore #788

Remove detection of files via import resolution, use static glob and npmignore #788

Conversation

jakebailey commented Oct 27, 2023 • edited Loading

TODO

jakebailey commented Oct 27, 2023 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

jakebailey commented Oct 31, 2023

sandersn left a comment

Choose a reason for hiding this comment

sandersn Nov 1, 2023

Choose a reason for hiding this comment

jakebailey Nov 1, 2023

Choose a reason for hiding this comment

jakebailey Nov 1, 2023

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

jakebailey commented Oct 27, 2023 •

edited

Loading

jakebailey commented Oct 27, 2023 •

edited

Loading