Using TypeScript for static typing by zkochan · Pull Request #357 · pnpm/pnpm

zkochan · 2016-09-11T11:30:35Z

I used TS because I have more experience with it, if someone will want to rewrite it to flow, I'll have no objections.

Still need to add a lot of types... I'll make the TypeScript configuration more strict once all the types will be there

zkochan · 2016-09-11T18:22:49Z

Wow, this line of code was so hard for me to understand:

if (err || srcPath === linkString) return cb(err)

from here

It can be both an error or a success!

rstacruz · 2016-09-12T14:12:08Z

if (err || srcPath === linkString) return cb(err)

Normally I write if (x || y) to mean: if x, or if it's not x but y...

In this case, it's like:

If it's an error...
...or if it's not an error, but srcPath === linkString...

Or more like:

Consider srcPath === linkString an error

But I can't actually remember what went on here, sorry :P

rstacruz · 2016-09-12T14:14:48Z

Oh alright, that wasn't entirely true, I think what happened here is:

    fs.readlink(dstPath, (err, linkString) => {
      if (err || srcPath === linkString) return cb(err)

pseudo code translation:

check out dstPath
if we get an error, throw an error
if it's a symlink to the correct path already (srcPath), it's a success

more verbosely:

    fs.readlink(dstPath, (err, linkString) => {
      if (err) return cb(err)
      if (srcPath === linkString) return cb()

I think I lifted this code off somewhere, haha

zkochan · 2016-09-12T14:15:22Z

In the end I've figured out the logic. Here's the outcome after the rewrite:

export default async function forceSymlink (srcPath: string, dstPath: string, type: SymlinkType) {
  debug(`${srcPath} -> ${dstPath}`)
  try {
    fs.symlinkSync(srcPath, dstPath, type)
    return
  } catch (err) {
    if ((<NodeJS.ErrnoException>err).code !== 'EEXIST') throw err

    const linkString = await fs.readlink(dstPath)
    if (srcPath === linkString) {
      return
    }
    await fs.unlink(dstPath)
    await forceSymlink(srcPath, dstPath, type)
  }
}

zkochan · 2016-09-12T14:17:00Z

The thing that confused me was that even on success, err was passed to cb. But the err is not defined on success. It was just visually confusing 😄

zkochan · 2016-09-12T14:21:04Z

By the way, this is not ready for merge yet. I have broken some logic with the last commit.
Till the refactor(*.ts): use async/await commit it is fine. The last commit I will fix in the evening. And I guess I'll work on even stricter rules in a separate pull request. To turn on TypeScript's strictNullChecks

PR #357, Ref #347

PR #357, Close #347

iamstarkov · 2016-09-12T19:54:18Z

+4,518 −1,386

huge job ✨

pnpm#349) * feat(reporter): add NDJSON reporting engine with pnpm:stage smoke test Introduces `pacquet-reporter` carrying the typed `LogEvent` enum, the no-`self` `Reporter` capability trait per pnpm#339, and `NdjsonReporter` / `SilentReporter` implementations. Adds the `--reporter=ndjson|silent` flag and threads a `R: Reporter` generic through `Install::run`, emitting `pnpm:stage` `importing_started` / `importing_done` to bracket the install — one channel wired end-to-end as a smoke test for the schema-mirror approach. The wire format mirrors what `@pnpm/core-loggers` defines and `@pnpm/cli.default-reporter` consumes under `pnpm --reporter=ndjson`. Engine only. The JS-reporter spawn-and-pipe (`--reporter=default`), the backfill of missing log events in already-ported code (pnpm#347), and the porting convention in AGENTS.md (pnpm#346) land in follow-up PRs. The default reporter is `silent` until the spawn-and-pipe exists, so user- facing output is unchanged. Refs pnpm#345. Design: pnpm#344. * docs(package-manager): name pnpm's `lockfileDir` and link findWorkspaceDir Upstream emits stage events with `prefix: lockfileDir` — the directory holding `pnpm-lock.yaml`, which collapses to the workspace root when a workspace is present. The previous comment described the *result* in pacquet's no-workspace world but didn't name the upstream concept or point at the helper to mirror once workspaces land. Replacing the comment with a TODO that links `findWorkspaceDir` makes the migration path explicit so the next person editing this site doesn't have to re-derive it. No behavior change. * fix(cli): make --reporter accepted on either side of the subcommand Adding `global = true` to the clap attribute lets users write either `pacquet --reporter=ndjson install` or `pacquet install --reporter=ndjson`, matching pnpm. Without it the flag is parsed by the top-level `CliArgs` only, so a sub-command-side use ("pacquet install --reporter=...") produced "unexpected argument '--reporter' found". * refactor(reporter,package-manager): apply review fixes from pnpm#349 Address feedback on the reporter PR. reporter: - Drop the dependency-injection issue link (pnpm#339) from `Reporter`, the recording-fake test, and the module-level docs; convention is visible from the code. - Wrap `bunyan` and `bole` references as markdown links. - Demote `Envelope`'s `#[serde(flatten)]` rationale to a regular `//` comment so private impl detail isn't part of `///` rustdoc. - Introduce `pub trait EnvVar` + `pub struct RealEnvVar` (reporter- local capability trait) and split `hostname` into a generic `resolve_hostname<E>` plus a non-generic cached production wrapper. Add four unit tests covering `HOSTNAME`, `COMPUTERNAME` fallback, precedence, and the unset case. - Switch the wire-shape test to `pipe-trait`; add `pipe-trait` to dev-dependencies. - Fix intra-link rule violation in the `SilentReporter` test doc. package-manager: - Tighten the install `prefix` extraction to fail loudly on non-UTF-8 / parentless manifest paths instead of silently emitting an empty string. Workspace-aware resolution tracked at pnpm#357. - Replace `concat!` YAML literals in two tests with `text_block_macros::text_block!`; add as dev-dependency. - Convert bare-prose identifiers in `install_emits_stage_events_bracketing_the_run` to intra-doc links; drop the recording-fake DI prose. Long inline `mod tests` extraction tracked at pnpm#358. * fix(reporter): drop private-item link from `EnvVar` public doc `EnvVar` is `pub` but `resolve_hostname` is private; rustdoc rejects the intra-link with `-D rustdoc::private-intra-doc-links` (the rule added to `CODE_STYLE_GUIDE.md` in pnpm#351). Rephrase the doc to talk about "env-dependent helpers" generally without naming the private function. * refactor(reporter): switch hostname to gethostname syscall + DI capability Address the second round of review feedback on pnpm#349. reporter: - Replace env-var-based hostname (HOSTNAME / COMPUTERNAME) with the real gethostname(2) syscall via the `gethostname` crate. The env-var approach was unsound: shells set `HOST` (zsh) or `HOSTNAME` (bash) as private variables that don't propagate to spawned binaries, so a packaged `pacquet` would have observed an empty hostname in most production environments. - Introduce `pub trait GetHostName` + `pub struct RealApi`. Real resolution lives in `RealApi::get_host_name`. Tests can supply their own impl. - Cache the production value in `static HOSTNAME: LazyLock<String> = LazyLock::new(RealApi::get_host_name)`. The wire emit path reads `&HOSTNAME` directly; the previous `hostname()` wrapper is no longer needed. - Drop the now-redundant `EnvVar` trait, `RealEnvVar`, `resolve_hostname`, and the four env-resolution unit tests (those scenarios no longer exist after the syscall switch). reporter test layout: - Move the tests module into its own file at `crates/reporter/src/tests.rs` per the unit-test-layout rule in `CODE_STYLE_GUIDE.md`. Drop `use super::*` in favor of explicit merged imports of just the items the tests touch. - Two new tests: `get_host_name_capability_is_mockable` (proves the trait dispatch works for fakes) and `real_api_returns_a_non_empty_host_name` (smoke-checks the real syscall path). Documentation style: - Restructure the workspace-prefix and stage-bracketing comments to drop em dashes per `CONTRIBUTING.md`'s writing-style guidance and to render `bunyan` as a markdown link. - Fix the `@pnpm/core-loggers` link to point at the package directory rather than its `src` subdirectory. Imports: - Replace seven occurrences of `pacquet_reporter::SilentReporter` qualified at the call site with a single `use pacquet_reporter:: SilentReporter` in the test module.

…#431) (pnpm#443) Stage 1 of pnpm/pacquet#299 / closes pnpm#431 (Stage 1 scope) / partially closes pnpm#357. - **New crate `pacquet-workspace`** — ports pnpm's `workspace/root-finder`, `workspace/workspace-manifest-reader`, `workspace/projects-reader`, and `workspace/project-manifest-reader` (pinned to [`94240bc046`](https://github.com/pnpm/pnpm/tree/94240bc046)). `find_workspace_dir` honours `NPM_CONFIG_WORKSPACE_DIR` (and its lowercase spelling, with empty-value fall-through), walks up to `pnpm-workspace.yaml`, and rejects misnamed variants (`pnpm-workspace.yml`, `.pnpm-workspace.yaml`, ...) with `BAD_WORKSPACE_MANIFEST_NAME`. Project enumeration glob-expands `packages:` via `wax`, always includes the workspace root (pnpm#1986), filters `**/node_modules/**` + `**/bower_components/**`, sorts lex by `rootDir`, and preserves the omitted-vs-explicit-empty distinction so `packages: []` enumerates only the root rather than falling back to the recursive `['.', '**']` default. - **Per-importer install path** — `SymlinkDirectDependencies` iterates every entry in `Lockfile.importers`, computes per-project `rootDir` and `node_modules/`, and uses `rootDir` as the `prefix` for `pnpm:root added` events (matching upstream's [`linkDirectDeps`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/direct-dep-linker/src/linkDirectDeps.ts#L131)). Install-wide events (`pnpm:stage`, `pnpm:summary`) still use `lockfileDir`, now resolved through `find_workspace_dir`. `MissingRootImporter` is gone — an empty importers map is a silent no-op. A custom `modulesDir` from `pnpm-workspace.yaml` propagates to every importer (the symlink stage reads `config.modules_dir.file_name()` rather than hard-coding `node_modules`). - **Path-anchoring consistency** — `Config::current` re-anchors `modules_dir` / `virtual_store_dir` to the workspace root when `pnpm-workspace.yaml` is found in an ancestor, mirroring pnpm v11's `pnpmConfig.dir = lockfileDir`. Running `pacquet install` from a workspace subdirectory no longer produces two inconsistent `node_modules` layouts (subdir virtual store vs workspace-root per-importer symlinks). `Config::current` also honours `NPM_CONFIG_WORKSPACE_DIR` so the env-var override drives both `find_workspace_dir` and the path anchors. `workspace_root` is threaded through `InstallFrozenLockfile` as a real `&Path` (no lossy-UTF-8 round-trip), so non-UTF-8 filenames survive the install layer intact. - **Lockfile `link:` support** — `ResolvedDependencySpec.version` is now an `ImporterDepVersion` enum (`Regular(PkgVerPeer) | Link(String)`), so a v9 workspace lockfile with `version: link:../shared` parses. The symlink stage resolves the link target against the importer's `rootDir` and points `node_modules/<name>` at the sibling project rather than the virtual store; `BuildModules`, `pacquet-real-hoist`, and the side-effects-cache write path all skip `Link` variants when collecting snapshot-key roots. - **Importer-key validation** — rejects absolute POSIX paths, Windows drive prefixes, backslash separators, `..` segments, and the empty string with a typed `UnsafeImporterPath` error, so a malformed (or hostile) lockfile cannot make `Path::join` create `node_modules` outside the workspace root. - **Symlink error propagation** — the prior `expect("symlink pkg")` is replaced with `try_for_each` + a `SymlinkDirectDependenciesError::SymlinkPackage` variant carrying the `importer_id`, `name`, and underlying `SymlinkPackageError`. A filesystem failure no longer crashes the rayon pool. - **Adds `wax = 0.7.0`** to `[workspace.dependencies]` for project globbing.

pnpm#349) * feat(reporter): add NDJSON reporting engine with pnpm:stage smoke test Introduces `pacquet-reporter` carrying the typed `LogEvent` enum, the no-`self` `Reporter` capability trait per pnpm#339, and `NdjsonReporter` / `SilentReporter` implementations. Adds the `--reporter=ndjson|silent` flag and threads a `R: Reporter` generic through `Install::run`, emitting `pnpm:stage` `importing_started` / `importing_done` to bracket the install — one channel wired end-to-end as a smoke test for the schema-mirror approach. The wire format mirrors what `@pnpm/core-loggers` defines and `@pnpm/cli.default-reporter` consumes under `pnpm --reporter=ndjson`. Engine only. The JS-reporter spawn-and-pipe (`--reporter=default`), the backfill of missing log events in already-ported code (pnpm#347), and the porting convention in AGENTS.md (pnpm#346) land in follow-up PRs. The default reporter is `silent` until the spawn-and-pipe exists, so user- facing output is unchanged. Refs pnpm#345. Design: pnpm#344. * docs(package-manager): name pnpm's `lockfileDir` and link findWorkspaceDir Upstream emits stage events with `prefix: lockfileDir` — the directory holding `pnpm-lock.yaml`, which collapses to the workspace root when a workspace is present. The previous comment described the *result* in pacquet's no-workspace world but didn't name the upstream concept or point at the helper to mirror once workspaces land. Replacing the comment with a TODO that links `findWorkspaceDir` makes the migration path explicit so the next person editing this site doesn't have to re-derive it. No behavior change. * fix(cli): make --reporter accepted on either side of the subcommand Adding `global = true` to the clap attribute lets users write either `pacquet --reporter=ndjson install` or `pacquet install --reporter=ndjson`, matching pnpm. Without it the flag is parsed by the top-level `CliArgs` only, so a sub-command-side use ("pacquet install --reporter=...") produced "unexpected argument '--reporter' found". * refactor(reporter,package-manager): apply review fixes from pnpm#349 Address feedback on the reporter PR. reporter: - Drop the dependency-injection issue link (pnpm#339) from `Reporter`, the recording-fake test, and the module-level docs; convention is visible from the code. - Wrap `bunyan` and `bole` references as markdown links. - Demote `Envelope`'s `#[serde(flatten)]` rationale to a regular `//` comment so private impl detail isn't part of `///` rustdoc. - Introduce `pub trait EnvVar` + `pub struct RealEnvVar` (reporter- local capability trait) and split `hostname` into a generic `resolve_hostname<E>` plus a non-generic cached production wrapper. Add four unit tests covering `HOSTNAME`, `COMPUTERNAME` fallback, precedence, and the unset case. - Switch the wire-shape test to `pipe-trait`; add `pipe-trait` to dev-dependencies. - Fix intra-link rule violation in the `SilentReporter` test doc. package-manager: - Tighten the install `prefix` extraction to fail loudly on non-UTF-8 / parentless manifest paths instead of silently emitting an empty string. Workspace-aware resolution tracked at pnpm#357. - Replace `concat!` YAML literals in two tests with `text_block_macros::text_block!`; add as dev-dependency. - Convert bare-prose identifiers in `install_emits_stage_events_bracketing_the_run` to intra-doc links; drop the recording-fake DI prose. Long inline `mod tests` extraction tracked at pnpm#358. * fix(reporter): drop private-item link from `EnvVar` public doc `EnvVar` is `pub` but `resolve_hostname` is private; rustdoc rejects the intra-link with `-D rustdoc::private-intra-doc-links` (the rule added to `CODE_STYLE_GUIDE.md` in pnpm#351). Rephrase the doc to talk about "env-dependent helpers" generally without naming the private function. * refactor(reporter): switch hostname to gethostname syscall + DI capability Address the second round of review feedback on pnpm#349. reporter: - Replace env-var-based hostname (HOSTNAME / COMPUTERNAME) with the real gethostname(2) syscall via the `gethostname` crate. The env-var approach was unsound: shells set `HOST` (zsh) or `HOSTNAME` (bash) as private variables that don't propagate to spawned binaries, so a packaged `pacquet` would have observed an empty hostname in most production environments. - Introduce `pub trait GetHostName` + `pub struct RealApi`. Real resolution lives in `RealApi::get_host_name`. Tests can supply their own impl. - Cache the production value in `static HOSTNAME: LazyLock<String> = LazyLock::new(RealApi::get_host_name)`. The wire emit path reads `&HOSTNAME` directly; the previous `hostname()` wrapper is no longer needed. - Drop the now-redundant `EnvVar` trait, `RealEnvVar`, `resolve_hostname`, and the four env-resolution unit tests (those scenarios no longer exist after the syscall switch). reporter test layout: - Move the tests module into its own file at `crates/reporter/src/tests.rs` per the unit-test-layout rule in `CODE_STYLE_GUIDE.md`. Drop `use super::*` in favor of explicit merged imports of just the items the tests touch. - Two new tests: `get_host_name_capability_is_mockable` (proves the trait dispatch works for fakes) and `real_api_returns_a_non_empty_host_name` (smoke-checks the real syscall path). Documentation style: - Restructure the workspace-prefix and stage-bracketing comments to drop em dashes per `CONTRIBUTING.md`'s writing-style guidance and to render `bunyan` as a markdown link. - Fix the `@pnpm/core-loggers` link to point at the package directory rather than its `src` subdirectory. Imports: - Replace seven occurrences of `pacquet_reporter::SilentReporter` qualified at the call site with a single `use pacquet_reporter:: SilentReporter` in the test module.

…#431) (pnpm#443) Stage 1 of pnpm/pacquet#299 / closes pnpm#431 (Stage 1 scope) / partially closes pnpm#357. - **New crate `pacquet-workspace`** — ports pnpm's `workspace/root-finder`, `workspace/workspace-manifest-reader`, `workspace/projects-reader`, and `workspace/project-manifest-reader` (pinned to [`94240bc046`](https://github.com/pnpm/pnpm/tree/94240bc046)). `find_workspace_dir` honours `NPM_CONFIG_WORKSPACE_DIR` (and its lowercase spelling, with empty-value fall-through), walks up to `pnpm-workspace.yaml`, and rejects misnamed variants (`pnpm-workspace.yml`, `.pnpm-workspace.yaml`, ...) with `BAD_WORKSPACE_MANIFEST_NAME`. Project enumeration glob-expands `packages:` via `wax`, always includes the workspace root (pnpm#1986), filters `**/node_modules/**` + `**/bower_components/**`, sorts lex by `rootDir`, and preserves the omitted-vs-explicit-empty distinction so `packages: []` enumerates only the root rather than falling back to the recursive `['.', '**']` default. - **Per-importer install path** — `SymlinkDirectDependencies` iterates every entry in `Lockfile.importers`, computes per-project `rootDir` and `node_modules/`, and uses `rootDir` as the `prefix` for `pnpm:root added` events (matching upstream's [`linkDirectDeps`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/direct-dep-linker/src/linkDirectDeps.ts#L131)). Install-wide events (`pnpm:stage`, `pnpm:summary`) still use `lockfileDir`, now resolved through `find_workspace_dir`. `MissingRootImporter` is gone — an empty importers map is a silent no-op. A custom `modulesDir` from `pnpm-workspace.yaml` propagates to every importer (the symlink stage reads `config.modules_dir.file_name()` rather than hard-coding `node_modules`). - **Path-anchoring consistency** — `Config::current` re-anchors `modules_dir` / `virtual_store_dir` to the workspace root when `pnpm-workspace.yaml` is found in an ancestor, mirroring pnpm v11's `pnpmConfig.dir = lockfileDir`. Running `pacquet install` from a workspace subdirectory no longer produces two inconsistent `node_modules` layouts (subdir virtual store vs workspace-root per-importer symlinks). `Config::current` also honours `NPM_CONFIG_WORKSPACE_DIR` so the env-var override drives both `find_workspace_dir` and the path anchors. `workspace_root` is threaded through `InstallFrozenLockfile` as a real `&Path` (no lossy-UTF-8 round-trip), so non-UTF-8 filenames survive the install layer intact. - **Lockfile `link:` support** — `ResolvedDependencySpec.version` is now an `ImporterDepVersion` enum (`Regular(PkgVerPeer) | Link(String)`), so a v9 workspace lockfile with `version: link:../shared` parses. The symlink stage resolves the link target against the importer's `rootDir` and points `node_modules/<name>` at the sibling project rather than the virtual store; `BuildModules`, `pacquet-real-hoist`, and the side-effects-cache write path all skip `Link` variants when collecting snapshot-key roots. - **Importer-key validation** — rejects absolute POSIX paths, Windows drive prefixes, backslash separators, `..` segments, and the empty string with a typed `UnsafeImporterPath` error, so a malformed (or hostile) lockfile cannot make `Path::join` create `node_modules` outside the workspace root. - **Symlink error propagation** — the prior `expect("symlink pkg")` is replaced with `try_for_each` + a `SymlinkDirectDependenciesError::SymlinkPackage` variant carrying the `importer_id`, `name`, and underlying `SymlinkPackageError`. A filesystem failure no longer crashes the rayon pool. - **Adds `wax = 0.7.0`** to `[workspace.dependencies]` for project globbing.

zkochan changed the title ~~Using TypeScript for status typing~~ Using TypeScript for static typing Sep 11, 2016

zkochan force-pushed the ts branch 3 times, most recently from 491c7be to 612c7ed Compare September 11, 2016 15:53

zkochan force-pushed the ts branch 4 times, most recently from 6d72577 to 8992189 Compare September 12, 2016 05:50

zkochan force-pushed the ts branch 2 times, most recently from 9f73e0b to 5ac7a7d Compare September 12, 2016 18:53

zkochan added 5 commits September 12, 2016 22:00

feat: rewrite to TypeScript

7b25ba2

PR #357, Ref #347

style: use tslint

3cf5c8b

PR #357, Ref #347

refactor(*.ts): noImplicitAny = true

2f26f61

PR #357, Ref #347

refactor(*.ts): noImplicitReturns = true

356b8b2

PR #357, Ref #347

refactor(*.ts): use async/await

261d968

PR #357, Close #347

zkochan force-pushed the ts branch from 5ac7a7d to 261d968 Compare September 12, 2016 19:04

zkochan merged commit 261d968 into master Sep 12, 2016

zkochan deleted the ts branch September 12, 2016 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Using TypeScript for static typing#357

Using TypeScript for static typing#357
zkochan merged 5 commits into
masterfrom
ts

zkochan commented Sep 11, 2016

Uh oh!

zkochan commented Sep 11, 2016 •

edited

Loading

Uh oh!

rstacruz commented Sep 12, 2016

Uh oh!

rstacruz commented Sep 12, 2016 •

edited

Loading

Uh oh!

zkochan commented Sep 12, 2016

Uh oh!

zkochan commented Sep 12, 2016 •

edited

Loading

Uh oh!

zkochan commented Sep 12, 2016

Uh oh!

iamstarkov commented Sep 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

zkochan commented Sep 11, 2016

Uh oh!

zkochan commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rstacruz commented Sep 12, 2016

Uh oh!

rstacruz commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan commented Sep 12, 2016

Uh oh!

zkochan commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan commented Sep 12, 2016

Uh oh!

iamstarkov commented Sep 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zkochan commented Sep 11, 2016 •

edited

Loading

rstacruz commented Sep 12, 2016 •

edited

Loading

zkochan commented Sep 12, 2016 •

edited

Loading