Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Aug 28, 2025

  • Initial analysis and plan for including nested package.json files
  • Include nested package.json files in generated packages
  • Update tests and snapshots for nested package.json inclusion
  • Address code review feedback:
    • Rename addFileIfDeclaration to addFileIfIncluded to better reflect its purpose
    • Exclude root package.json from getFiles() as it's handled specially by the publisher
    • Update test expectations to not include root package.json
    • Add explicit test assertion that root package.json is excluded
    • Update publisher test snapshots to reflect new content hash
    • Remove incorrectly committed package-lock.json (repo uses pnpm)

Changes Made

Problem Addressed

Some DefinitelyTyped packages need nested package.json files to control module type per subfolder (e.g., src as ESM and lib as CJS). The file enumeration logic needed to be updated to include these nested configuration files while excluding the root package.json which is handled specially.

Solution

Modified the getFiles() function in packages/definitions-parser/src/lib/definition-parser.ts:

  • Renamed function from addFileIfDeclaration to addFileIfIncluded for clarity
  • Updated condition to include nested package.json files: path.endsWith('/package.json')
  • Excluded root package.json (handled specially by publisher)

Testing

  • Added comprehensive test case verifying nested package.json files are included
  • Updated existing test expectations to exclude root package.json
  • Updated publisher snapshots to reflect new content hash
  • All tests pass for the new functionality

This enables DefinitelyTyped packages to properly support mixed module systems within a single package.

This pull request was created as a result of the following prompt from Copilot chat.

Summary
Some DefinitelyTyped packages need nested package.json files to control module type per subfolder (e.g., src as ESM and lib as CJS). Today, DefinitelyTyped-tools does not include such nested package.json files in generated packages because file enumeration only includes declaration files. As a result, these nested package.json files are omitted from the output directory and tarballs.

Goal

  • Ensure nested package.json files inside a typings package (types//...) are included in generated packages and therefore end up in published tarballs.
  • Preserve existing behavior that the root package.json is synthesized by the publisher and should not be copied from DT.
  • Maintain exclusion of older-version directories (e.g., v1, v0.61) as today.

Background

  • packages/definitions-parser/src/lib/definition-parser.ts:getFiles builds the list of files to copy, but currently only pushes declaration files via isDeclarationPath.
  • packages/publisher/src/generate-packages.ts writes package.json/README/LICENSE, then copies all files returned by TypingsData.getFiles(). Therefore, extending getFiles to include nested package.json files will naturally flow into publisher output.
  • getContentHash in TypingsData uses [...getFiles(), "package.json"]. Once nested package.json files are included in getFiles, changes to them will affect content hashes (desired).

Requirements / Acceptance Criteria

  1. Definitions parser includes nested package.json files:

    • Update getFiles so it collects declaration files and any nested package.json files (path contains a slash and ends with "/package.json").
    • Do not include the root package.json from the package root (types//package.json), since publisher generates that.
    • Continue to skip older-version directories entirely when enumerating files.
  2. Tests (TDD):

    • definitions-parser test: create a mock package with src/package.json and lib/package.json, assert TypingsData.getFiles() includes those but NOT the root package.json.
    • publisher test: ensure generatePackages copies nested package.json files to the output directory for a sample package.
  3. No changes to publishing logic besides relying on extended getFiles.

Proposed Implementation Details

  • In packages/definitions-parser/src/lib/definition-parser.ts, modify getFiles:
    • Add a helper isNestedPackageJson(path: string) that returns true if path ends with "/package.json".
    • Change addFileIfDeclaration -> addFileIfIncluded to push files when isDeclarationPath(path) || isNestedPackageJson(path).
    • Keep the logic that skips olderVersionDirectories when iterating the root.
  • Write tests:
    • packages/definitions-parser/test/definition-parser.test.ts: add a test "includes nested package.json files but not the root package.json" using createMockDT and AllPackages.
    • packages/publisher/test/generate-packages.test.ts: add a test that after generatePackages, the outputDirectory contains src/package.json and lib/package.json for a mock package; avoid asserting on tarball contents, just the output folder is fine.

Edge Cases

  • Versioned subdirectories (vX or vX.Y): nested package.json within those should remain excluded because the entire older-version directory is skipped today.
  • tsVersion directories (tsX.Y): They are not part of olderVersionDirectories; if nested package.json exists under a tsVersion directory, it should be included as long as it's not the root.

Out-of-Scope

  • Any change to how .npmignore is used or any broader file inclusion policy.

Testing Notes

  • Run test suites for definitions-parser and publisher.
  • Verify no other tests regress; in particular, getContentHash expectations should still hold.

Deliverables

  • Code changes to definitions-parser getFiles with clear comments.
  • New tests in definitions-parser and publisher.
  • Changelog entries if required by the package (follow repo conventions).

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Include nested package.json files in generated @types packages (support module type per subfolder) Include nested package.json files in generated DefinitelyTyped packages Aug 28, 2025
@Copilot Copilot AI requested a review from andrewbranch August 28, 2025 23:10
Copilot finished work on behalf of andrewbranch August 28, 2025 23:10
@andrewbranch
Copy link
Member

@copilot see my review above

Copy link
Author

Copilot AI commented Aug 28, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • functionscdn.azureedge.net
    • Triggering command: node lib/install.js (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@Copilot Copilot AI requested a review from andrewbranch August 28, 2025 23:51
Copilot finished work on behalf of andrewbranch August 28, 2025 23:51
@andrewbranch
Copy link
Member

@copilot you commited a package-lock.json when we use pnpm

Copy link
Author

Copilot AI commented Aug 29, 2025

@copilot you commited a package-lock.json when we use pnpm

Fixed in commit 0a8f4c4. Removed the incorrectly committed package-lock.json file since this repository uses pnpm, not npm.

Copilot finished work on behalf of andrewbranch August 29, 2025 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants