PNPM workspaces #663

sandersn · 2023-04-10T18:10:31Z

Current progress:

builds
tests pass
I manually tested that dtslint succeeds on
- 3box (some random package)
- adeira__js (scoped package)
- gulp/v3 (old-version package)
- har-format (typesVersions package)
- react
- node
  react and node fail with errors now, which I need to look at.
dtslint-runner parses all definitions on real DT.
dtslint-runner succeeds in checking parse results.
some packages fail, mostly with npm-naming and expect (I'm halfway through a local run)

TODO:

DT monorepo conversion TODOs #751
Address a bunch of old TODOs
Locally, force-publish every package and compare the output to a force-publish based on the present-day code.

linting starts and then fails because tests can't resolve their types packages

I'm at the point of checking `version` and I think I need to copy code from definition-parser maybe. Visions of semver parsing...

It's already written! Also make deepEqual traverse ojbects as well as arrays.

Show all unused files at once, not just the first unused one.

Deletes a whole bunch of stuff and simplifies or reworks others. I noticed that we still want to walk the file looking for globals SO I have to put back in huge chunks of code I took out. Creating a commit so I can diff on github.

Everything builds now!

It's only one line of code now

Also all (?) the infrastructure needed to support it.

packages/dtslint-runner/src/prepareAffectedPackages.ts

self-references sometimes result in doubled 'node_modules/@types/self-package-name' prefixes.

Lots of code churn and probable bugs, but no real functionality change right now. In the future this makes it much easier to mimic real ES module layouts.

packages/header-parser/src/index.ts

Plus update typescript again to match DT

packages/header-parser/src/index.ts

2. Simplify and reduce JSON parsing in dtslint-runner. Pass around AllPackages object instead of writing it to disk and reading it over and over. 3. Sort AllPackages object before returning it in addition to before writing it to disk.

Still not 100% semver, but the common paths now use it.

Only a few fixes needed this time!

packages/definitions-parser/src/git.ts

jakebailey

Okay overall seems good, with some general notes of:

Seems like there may be some inconsistency in the use of .typesDirectoryName and .name
I think we should be able to drop parsimmon as a dep for the header parser now? May be worth running depcheck on the repo (or knip)

packages/definitions-parser/src/lib/definition-parser.ts

packages/definitions-parser/src/lib/utils.ts

packages/definitions-parser/src/get-affected-packages.ts

packages/definitions-parser/src/git.ts

packages/publisher/src/generate-packages.ts

packages/publisher/src/publish-registry.ts

jakebailey · 2023-10-16T18:49:37Z

packages/retag/src/index.ts

@@ -139,8 +139,8 @@ export async function fetchTypesPackageVersionInfo(
  if (info.deprecated) {
    // https://github.com/DefinitelyTyped/DefinitelyTyped/pull/22306
    assert(
-      pkg.name === "angular-ui-router" || pkg.name === "ui-router-extras",
-      `Package ${pkg.name} has been deprecated, so we shouldn't have parsed it. Was it re-added?`
+      pkg.typesDirectoryName === "angular-ui-router" || pkg.typesDirectoryName === "ui-router-extras",


Not sure if typesDirectoryName includes a version or not, but maybe this should just be checking === "@types/angular-ui-router"? Not sure what this check is doing at this point though.

Looking through the referenced PR, it looks like these were a manual package rename, or maybe manual deprecation, and then later somebody tried to re-add the types.

It seems like ancient history that we shouldn't need to check for, but I also don't feel like removing it and testing retag right now.

packages/utils/src/assertions.ts

packages/utils/src/io.ts

…ad code

1. Patch version is now .9999 not .99999 2. contributors -> owners 3. typeScriptVersion -> minimumTypeScriptVersion Note that the publisher should still produce a package.json with a contributor property.

sandersn · 2023-10-13T18:26:20Z

packages/utils/src/assertions.ts

@@ -34,3 +34,20 @@ export function assertSorted<T>(
  }
  return a;
 }
+
+export function deepEquals(expected: unknown, actual: unknown): boolean {
+  if (expected instanceof Array) {


shouldn't this be Array.isArray?

sandersn · 2023-10-13T18:29:00Z

packages/utils/src/assertions.ts

+    );
+  } else if (typeof expected === "object") {
+    for (const k in expected) {
+      if (!deepEquals((expected as any)[k], (actual as any)[k])) {


this will crash when actual === null. Or, actually, undefined, because it doesn't check whether typeof actual === "object"

packages/dtslint-runner/src/prepareAllPackages.ts

packages/header-parser/src/index.ts

packages/utils/src/io.ts

packages/definitions-parser/src/lib/definition-parser.ts

sandersn · 2023-10-16T22:18:30Z

packages/definitions-parser/src/git.ts

  for (const diff of diffs) {
+    if (diff.status !== "D") continue;


It will test things that might not need to be tested (but since they're complicated enough that git didn't understand, probably should be tested.) That's all that gitDeletions is used for now.

packages/definitions-parser/src/git.ts

packages/definitions-parser/src/get-affected-packages.ts

jakebailey · 2023-10-16T23:11:48Z

I am not sure where, but some part of the code's linting is still suggesting putting stuff in the header:

Error in wicg-file-system-access
Error: /home/runner/work/DefinitelyTyped/DefinitelyTyped/DefinitelyTyped/types/wicg-file-system-access/index.d.ts:1:1
ERROR: 1:1  npm-naming  Declaration file must have a matching npm package.
To resolve this error, either:
1. Change the name to match an npm package.
2. Add a Definitely Typed header with the first line


// Type definitions for non-npm package wicg-file-system-access-browser

Add -browser to the end of your name to make sure it doesn't conflict with existing npm packages.
If you won't fix this error now or you think this error is wrong,
you can disable this check by adding the following options to your project's tslint.json file under "rules":

    "npm-naming": false
 See: https://github.com/microsoft/DefinitelyTyped-tools/blob/master/packages/dtslint/docs/npm-naming.md

    at testTypesVersion (/home/runner/work/DefinitelyTyped/DefinitelyTyped/DefinitelyTyped-tools/packages/dtslint/src/index.ts:218:11)
    at runTests (/home/runner/work/DefinitelyTyped/DefinitelyTyped/DefinitelyTyped-tools/packages/dtslint/src/index.ts:186:7)

jakebailey · 2023-10-16T23:23:53Z

yeah dts-critic's checkNpm still says this.

… comment

To correctly allow/disallow types/typesVersion in tsconfig

packages/definitions-parser/src/git.ts

andrewbranch · 2023-10-17T17:02:38Z

packages/definitions-parser/src/git.ts

+    if ("errors" in deleteds) errors.push(...deleteds.errors);
+    else
+      for (const deleted of deleteds.ok as NotNeededPackage[]) {
+        errors.push(...(await checkNotNeededPackage(deleted)));


Not proposing to change it now, but it’s really weird that this get* function is checking something as a side effect. It’s also weird that a function that’s supposed to “get affected packages” takes a parameter for whether you want... the affected packages... or all of them 🤔

I also just noticed that selection can be a regex, and that there's code to run only packages that match it.

Moments later: This code is only called with "affected"; "all" and regexes are unused. I deleted the dead code.

andrewbranch

I've looked pretty thoroughly at

definitions-parser
header-parser

and personally tested publisher pretty thoroughly. I haven't looked closely at dtslint, dtslint-runner, or eslint-plugin, but you've been testing those on DT as we go. So, I think I feel pretty good.

jakebailey

Seems good to me now, with the one comment about whether or not we should suggest a non-conflicting name in the new lint message.

jakebailey · 2023-10-17T17:40:44Z

packages/dts-critic/index.ts

-// Type definitions for non-npm package ${name}-browser
-
-Add -browser to the end of your name to make sure it doesn't conflict with existing npm packages.`,
+2. Add \`"nonNpm": true\` to the package.json to indicate that this is not an npm package.


Should we still suggest the -browser suffix as an example?

It's kind of a separate question from this PR. There are enough non-node environments now that we might want to explain that in the message, but it makes it even more complicated.

Yeah, we can do this later. -browser leads to people tacking -browser to their name to silence the lint even if it doesn't make any sense... Saw that once already last week.

Yeah, I felt that advice was very outdated at best

jakebailey

Seems good to me now, with the one comment about whether or not we should suggest a non-conflicting name in the new lint message.

jakebailey · 2023-10-17T17:46:51Z

Also, are we requiring nonNpmDescription when nonNpm is specified? I think before you had to because it'd use the text from the header, and we probably still want that?

sandersn · 2023-10-17T17:59:45Z

nonNpmDescription is required when nonNpm is specified, and not allowed when it is not.

It has a bunch of dead code!

sandersn added 15 commits March 28, 2023 16:18

remove baseUrl/typeRoots

b144b97

linting starts and then fails because tests can't resolve their types packages

WIP dtslint checks

689ccfc

I'm at the point of checking `version` and I think I need to copy code from definition-parser maybe. Visions of semver parsing...

check version in package.json

85f0532

update to ts@next to match DT

2dbbcee

Switch from no-outside-dependencies to import/no-extraneous-dependencies

683cbf2

It's already written! Also make deepEqual traverse ojbects as well as arrays.

Fix used-files calculation and improve error output

2a62b4b

Show all unused files at once, not just the first unused one.

allow all @types deps

9ffae5f

Mostly done switching to package.json dep/devDeps

55bde30

Deletes a whole bunch of stuff and simplifies or reworks others. I noticed that we still want to walk the file looking for globals SO I have to put back in huge chunks of code I took out. Creating a commit so I can diff on github.

Restore code for finding global values in source

b19ebf4

Everything builds now!

definitions-parser tests pass

3b866d1

*now* definitions-parser tests all pass

c30e50a

Make generate-packages tests pass

ffc9fc4

npm i -> pnpm i (compiles but hasn't run yet)

da605a2

Delete installDependencies entirely

3d5cd40

It's only one line of code now

Remove pathMapping check in definition-parser

a336a4e

Also all (?) the infrastructure needed to support it.

sandersn requested a review from jakebailey April 10, 2023 18:10

jakebailey reviewed Apr 10, 2023

View reviewed changes

packages/dtslint-runner/src/prepareAffectedPackages.ts Outdated Show resolved Hide resolved

sandersn added 3 commits April 10, 2023 12:53

slice multiple prefixes from resolved filenames

ecac2cc

self-references sometimes result in doubled 'node_modules/@types/self-package-name' prefixes.

add debug entrypoint for dtslint-runner

39d5fe7

Switch from index.d.ts header to package.json

72c0633

Lots of code churn and probable bugs, but no real functionality change right now. In the future this makes it much easier to mimic real ES module layouts.

github-advanced-security bot found potential problems Apr 14, 2023

View reviewed changes

packages/header-parser/src/index.ts Fixed Show fixed Hide fixed

Require either contributor url XOR githubUsername

2c80d98

Plus update typescript again to match DT

jakebailey reviewed Apr 17, 2023

View reviewed changes

packages/header-parser/src/index.ts Outdated Show resolved Hide resolved

Switch to reporting errors at end, not throwing one at a time

05fcf43

github-advanced-security bot found potential problems Apr 17, 2023

View reviewed changes

packages/header-parser/src/index.ts Fixed Show fixed Hide fixed

sandersn added 3 commits April 17, 2023 11:53

Switch to more semver

7bca052

Still not 100% semver, but the common paths now use it.

Package publication works now.

1749d27

Only a few fixes needed this time!

DanielRosenwasser mentioned this pull request May 17, 2023

TypeScript 5.2 Iteration Plan microsoft/TypeScript#54298

Closed

DanielRosenwasser mentioned this pull request Aug 23, 2023

TypeScript 5.3 Iteration Plan microsoft/TypeScript#55486

Open

make dtslint example files more generic

12b8ddc

sandersn mentioned this pull request Oct 13, 2023

DT monorepo conversion TODOs #751

Open

Edits from self-review of PR

beb0355

jakebailey reviewed Oct 16, 2023

View reviewed changes

packages/definitions-parser/src/git.ts Show resolved Hide resolved

sandersn added 3 commits October 16, 2023 10:19

npmNaming: report all errors always

9266a75

dtslint-runner --clone SHA runs pnpm install once again

1095384

fix direction of clone check

4aba26b

jakebailey reviewed Oct 16, 2023

View reviewed changes

andrewbranch and others added 5 commits October 16, 2023 13:14

Remove globals, make praser not crash on empty directories, remove de…

102c3f3

…ad code

Remove obsolete test

945114b

Finish removing globals stuff

b3509c5

Package.json: change properties and patch version

86a0736

1. Patch version is now .9999 not .99999 2. contributors -> owners 3. typeScriptVersion -> minimumTypeScriptVersion Note that the publisher should still produce a package.json with a contributor property.

address PR comments

577b7da

sandersn commented Oct 16, 2023

View reviewed changes

andrewbranch and others added 4 commits October 16, 2023 17:04

Update dts-critic message to reference package.json instead of header…

2bfab35

… comment

dts-critic correctly passes typesVersions to header-parser

2904537

To correctly allow/disallow types/typesVersion in tsconfig

allow 0.0 versions to mismatch npm again

f1c69db

get-affected-packages: check for dependents by package name

b4e40d1

andrewbranch reviewed Oct 17, 2023

View reviewed changes

andrewbranch approved these changes Oct 17, 2023

View reviewed changes

jakebailey approved these changes Oct 17, 2023

View reviewed changes

sandersn added 2 commits October 17, 2023 11:05

dtslint-runner: Simplify getAffectedPackagesFromDiff

1e7bb28

It has a bunch of dead code!

Add changeset for shipping

4a24663

sandersn merged commit 024c3e7 into master Oct 17, 2023
6 checks passed

sandersn deleted the pnpm-workspaces branch October 17, 2023 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PNPM workspaces #663

PNPM workspaces #663

sandersn commented Apr 10, 2023 •

edited by andrewbranch

Loading

jakebailey left a comment

jakebailey Oct 16, 2023

sandersn Oct 16, 2023

sandersn Oct 13, 2023

sandersn Oct 13, 2023

sandersn Oct 16, 2023

jakebailey commented Oct 16, 2023

jakebailey commented Oct 16, 2023

andrewbranch Oct 17, 2023

sandersn Oct 17, 2023

sandersn Oct 17, 2023

andrewbranch left a comment

jakebailey left a comment

jakebailey Oct 17, 2023

sandersn Oct 17, 2023

jakebailey Oct 17, 2023

andrewbranch Oct 17, 2023

jakebailey left a comment

jakebailey commented Oct 17, 2023

sandersn commented Oct 17, 2023

		for (const diff of diffs) {
		if (diff.status !== "D") continue;

PNPM workspaces #663

PNPM workspaces #663

Conversation

sandersn commented Apr 10, 2023 • edited by andrewbranch Loading

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented Oct 16, 2023

jakebailey commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

jakebailey commented Oct 17, 2023

sandersn commented Oct 17, 2023

sandersn commented Apr 10, 2023 •

edited by andrewbranch

Loading