Revert unapproved knip and moduleResolution PRs#299
Conversation
|
To be clear: the knip and moduleResolution work is valuable and should come back. The issue is sequencing and review. Once the Mocha migration (#266) lands and the test infrastructure is homogenized, we can re-land these as focused, reviewable PRs — one concern per PR, each properly approved. That's the right order: stabilize the test foundation first, then layer infrastructure changes on top. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR removes distributed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -1,29 +1,30 @@ | |||
| const pluginJs = require('@eslint/js') | |||
| const tseslint = require('typescript-eslint') | |||
| import globals from "globals"; | |||
| import { removeNode } from './disposal' | ||
| import { tagNameLower } from './info' | ||
| import * as domData from './data' | ||
| import options from '../options' |
|
@phillipc I'll look at knip & the other PR right after jasmine is out of the repo |
There was a problem hiding this comment.
Pull request overview
This PR reverts two previously merged but unapproved changesets that introduced knip, TypeScript config/module-resolution changes, d.ts tooling changes, and widespread package export/type-surface edits that conflicted with ongoing work and caused regressions.
Changes:
- Reverts TypeScript configuration/tooling changes (module resolution, d.ts generation approach, removal of
sync-dtstooling). - Removes
knipconfiguration and the dependency-cycle GitHub Actions workflow. - Restores/adjusts multiple package manifests and source exports/types across many packages.
Reviewed changes
Copilot reviewed 61 out of 64 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Restores non-bundler module resolution; toggles TS compiler options. |
| tsconfig.dts.json | Changes declaration emit configuration to a single bundled outFile. |
| tools/sync-dts.mjs | Removes the declaration sync script previously used by make dts. |
| tools/repackage.mjs | Updates standardized package.json shape (removes types export/files). |
| packages/utils/src/options.ts | Switches option typings to @tko/provider / @tko/builder types. |
| packages/utils/src/interfaces.ts | Removes local shared type definitions (ProviderBase/KnockoutUtils/etc). |
| packages/utils/src/index.ts | Removes re-export of deleted interfaces.ts types. |
| packages/utils/src/dom/virtualElements.ts | Adds options import and new hasBindingValue export. |
| packages/utils/package.json | Removes types/ packaging and adds tslib. |
| packages/utils.parser/package.json | Removes types/ packaging and adds tslib. |
| packages/utils.jsx/src/JsxObserver.ts | Adds default export for JsxObserver. |
| packages/utils.jsx/src/index.ts | Switches JsxObserver re-export to default-based form. |
| packages/utils.jsx/package.json | Removes types/ packaging and adds tslib. |
| packages/utils.functionrewrite/package.json | Removes types/ packaging and adds tslib. |
| packages/utils.component/package.json | Removes types/ packaging and adds tslib. |
| packages/provider/src/Provider.ts | Removes dependency on @tko/utils ProviderBase types; adjusts class/type defs. |
| packages/provider/src/BindingHandlerObject.ts | Loosens method typing. |
| packages/provider/package.json | Removes types/ packaging; changes dependencies (notably). |
| packages/provider.virtual/package.json | Removes types/ packaging and adds tslib. |
| packages/provider.native/package.json | Removes types/ packaging; changes dependencies (notably). |
| packages/provider.mustache/src/mustacheParser.ts | Exports parsing generators (innerParse, parseOuterMatch). |
| packages/provider.mustache/src/AttributeMustacheProvider.ts | Adjusts direct binding helper signature/return. |
| packages/provider.mustache/package.json | Removes types/ packaging; adjusts dependencies. |
| packages/provider.multi/package.json | Removes types/ packaging and adds tslib. |
| packages/provider.databind/package.json | Removes types/ packaging and adds tslib. |
| packages/provider.component/spec/customElementBehaviors.ts | Updates observable imports used in specs. |
| packages/provider.component/package.json | Removes types/ packaging and adds tslib. |
| packages/provider.bindingstring/package.json | Removes types/ packaging and adds tslib. |
| packages/provider.attr/package.json | Removes types/ packaging and adds tslib. |
| packages/observable/src/Subscription.ts | Converts Subscription to a default export. |
| packages/observable/src/subscribable.ts | Updates Subscription import to match default export. |
| packages/observable/src/observableArray.ts | Exposes MaybeObservableArray type. |
| packages/observable/src/index.ts | Removes exported Subscription type from the public entrypoint. |
| packages/observable/src/extenders.ts | Changes types/imports; exports extender functions. |
| packages/observable/package.json | Removes types/ packaging and adds tslib. |
| packages/lifecycle/src/LifeCycle.ts | Adjusts Observable import form. |
| packages/lifecycle/package.json | Removes types/ packaging and adds tslib. |
| packages/filter.punches/package.json | Removes types/ packaging and adds tslib. |
| packages/computed/src/proxy.ts | Removes @internal markers on exported helpers. |
| packages/computed/src/computed.ts | Exposes MaybeComputed type. |
| packages/computed/package.json | Removes types/ packaging and adds tslib. |
| packages/builder/src/Builder.ts | Inlines/expands utility typings; updates Knockout instance type composition. |
| packages/builder/package.json | Removes types/ packaging; adjusts dependencies (notably). |
| packages/binding.template/src/nativeTemplateEngine.ts | Adjusts TemplateOptions import form. |
| packages/binding.template/spec/nativeTemplateEngineBehaviors.ts | Updates observable imports used in specs. |
| packages/binding.template/package.json | Removes types/ packaging and adds tslib. |
| packages/binding.if/package.json | Removes types/ packaging; adjusts dependencies. |
| packages/binding.foreach/package.json | Removes types/ packaging; adjusts dependencies. |
| packages/binding.core/package.json | Removes types/ packaging and adds tslib. |
| packages/binding.component/package.json | Removes types/ packaging; adjusts dependencies (notably). |
| packages/bind/src/bindingEvent.ts | Changes Subscription typing import to a deep import. |
| packages/bind/src/bindingContext.ts | Adjusts Knockout instance typing and callback import form. |
| packages/bind/src/applyBindings.ts | Changes computed type/value imports and Provider typing. |
| packages/bind/package.json | Removes types/ packaging; adds @tko/provider + tslib. |
| package.json | Removes knip devDependency; adds globals devDependency. |
| package-lock.json | Updates lockfile for removed/added dependencies (notably knip removal). |
| Makefile | Removes knip targets; changes dts generation command. |
| knip.json | Removes knip configuration file. |
| eslint.config.js | Converts ESLint config to ESM-style syntax and adjusts ignores/rules. |
| builds/reference/src/common.ts | Restores CommonJS module.exports assignment. |
| builds/reference/package.json | Removes types/ packaging and adds tslib. |
| builds/knockout/package.json | Removes types/ packaging and adds tslib. |
| .gitignore | Stops ignoring per-package types folders and removes .codex ignore. |
| .github/workflows/dependency-cycle.yml | Removes knip-based dependency cycle workflow. |
|
|
||
| import { computed } from '@tko/computed' | ||
| import type { Observable } from '@tko/observable' | ||
| import { Observable } from '@tko/observable' |
There was a problem hiding this comment.
Observable is exported from @tko/observable as a type-only export, so importing it as a value (import { Observable }) will fail type-checking (and may generate an unused runtime import). Switch this to a type-only import.
| import { Observable } from '@tko/observable' | |
| import type { Observable } from '@tko/observable' |
| import type { Observable } from '@tko/observable' | ||
|
|
||
| import { type Computed, computed } from '@tko/computed' | ||
| import { Computed, computed } from '@tko/computed' |
There was a problem hiding this comment.
Computed is a type-only export from @tko/computed (see packages/computed/src/index.ts). Importing it as a value will cause a TypeScript error. Use a type-only import for Computed and keep computed as the value import.
| import { Computed, computed } from '@tko/computed' | |
| import { computed } from '@tko/computed' | |
| import type { Computed } from '@tko/computed' |
| import { contextAncestorBindingInfo } from './bindingEvent' | ||
|
|
||
| import type { BindingContextExtendCallback } from './applyBindings' | ||
| import { BindingContextExtendCallback } from './applyBindings' |
There was a problem hiding this comment.
BindingContextExtendCallback is exported as a type-only export from ./applyBindings. Importing it as a value will fail type-checking. Change this to import type { BindingContextExtendCallback } ....
| import { BindingContextExtendCallback } from './applyBindings' | |
| import type { BindingContextExtendCallback } from './applyBindings' |
| @@ -1,5 +1,5 @@ | |||
| import { makeArray, parseHtmlFragment } from '@tko/utils' | |||
| import { templateEngine, type TemplateOptions } from './templateEngine' | |||
| import { templateEngine, TemplateOptions } from './templateEngine' | |||
There was a problem hiding this comment.
TemplateOptions is a TypeScript interface (type-only). Importing it as a value (import { ... TemplateOptions }) will fail type-checking. Use import type { TemplateOptions } (and keep templateEngine as the value import).
| import { templateEngine, TemplateOptions } from './templateEngine' | |
| import { templateEngine } from './templateEngine' | |
| import type { TemplateOptions } from './templateEngine' |
| import { applyBindings } from '@tko/bind' | ||
|
|
||
| import { observable, type ObservableArray, observableArray } from '@tko/observable' | ||
| import { observable, ObservableArray, observableArray } from '@tko/observable' |
There was a problem hiding this comment.
ObservableArray is exported from @tko/observable as a type-only export, so importing it as a runtime value will fail type-checking. This should remain a type-only import (type ObservableArray).
| import { observable, ObservableArray, observableArray } from '@tko/observable' | |
| import { observable, type ObservableArray, observableArray } from '@tko/observable' |
| "dependencies": { | ||
| "@tko/bind": "^4.0.0", | ||
| "@tko/lifecycle": "^4.0.0", | ||
| "@tko/observable": "^4.0.0", | ||
| "@tko/provider": "^4.0.0", | ||
| "@tko/utils": "^4.0.0", | ||
| "@tko/utils.component": "^4.0.0", | ||
| "@tko/utils.jsx": "^4.0.0", | ||
| "@tko/provider.native": "^4.0.0" | ||
| "tslib": "^2.2.0" | ||
| }, |
There was a problem hiding this comment.
@tko/binding.component source imports NativeProvider from @tko/provider.native (see src/componentBinding.ts), but @tko/provider.native is no longer listed in dependencies. This will break runtime for consumers. Add @tko/provider.native back to dependencies (or adjust the implementation to not require it).
| import { domData } from '@tko/utils' | ||
| import { subscribable } from '@tko/observable' | ||
| import type { Subscribable, SubscriptionCallback, Subscription } from '@tko/observable' | ||
| import type { Subscribable, SubscriptionCallback } from '@tko/observable' | ||
| import type Subscription from '@tko/observable/src/Subscription' | ||
|
|
There was a problem hiding this comment.
This uses a deep import into another package’s source tree (@tko/observable/src/Subscription). With package exports and publish-time file lists, this path is very likely not resolvable for consumers generating types. Prefer importing Subscription from the public @tko/observable entrypoint (re-export the type there if needed) or avoid naming the concrete type in the signature.
| import { tagNameLower } from './info' | ||
| import * as domData from './data' | ||
| import options from '../options' | ||
|
|
There was a problem hiding this comment.
options is imported but never used in this file, which adds noise and can trigger unused-import checks in stricter toolchains. Remove the import or use it if it’s intended for side effects.
| $(NODE) tools/sync-dts.mjs .dts-tmp packages,builds/reference | ||
| rm -rf .dts-tmp | ||
| $(NPX) tsc --build tsconfig.dts.json | ||
|
|
There was a problem hiding this comment.
The knip target was removed, but repo docs (e.g. AGENTS.md lists make knip) still reference it, which will confuse contributors/CI usage. Either keep a stub knip target (that explains it was reverted) or update the documentation in the same PR to match the new Makefile targets.
| knip: | |
| @echo "The 'knip' target was removed/reverted and is kept here only as a compatibility stub." | |
| @echo "Please use the current Makefile targets documented for this repository instead." |
| import { Provider } from '@tko/provider' | ||
| import type { KnockoutInstance } from '@tko/builder' | ||
|
|
There was a problem hiding this comment.
Provider is only used as a type annotation in this file. Importing it as a value (import { Provider } ...) can introduce an unnecessary runtime dependency/side effects in the emitted JS depending on the build pipeline. Prefer import type { Provider } from '@tko/provider' here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dae7816ab2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "exclude": [ | ||
| "node_modules", | ||
| "builds/knockout", | ||
| "builds", | ||
| "docs", | ||
| "tools", | ||
| "**/spec/**", | ||
| "tko.io" | ||
| "**/spec/**" |
There was a problem hiding this comment.
This change drops tko.io from tsconfig.dts.json's exclude list, so make dts now picks up tko.io/src/**/*.ts (for example content.config.ts) during declaration generation. The root workspace only installs packages/* and builds/*, and the docs app depends on Astro-specific modules, so declaration generation fails on missing docs-only imports instead of producing library typings. Please add tko.io back to exclude (or use a narrow include) so the dts build only covers publishable library code.
Useful? React with 👍 / 👎.
Summary
Reverts two PRs that were merged without required review approval:
pc/switch_moduleResolution) — changedmoduleResolutionto"bundler", addedtsconfig.dts.json,sync-dts.mjs, modified package.json exports across all packagesAuge19/add_knip) — added knip linter, enabledverbatimModuleSyntax, createdinterfaces.ts, changed exports across packages, modified source files in 12+ packagesWhy revert
moduleResolutionand type exports in ways that conflict with the migration.JsxObserverhas type errors on main right now from the knip PR's type hierarchy changes.verbatimModuleSyntax, type refactoring, and export changes into one 56-file PR. Pc/switch module resolution #289 bundles moduleResolution, dts generation, and package.json export changes into one 40-file PR.Path forward
Both PRs contain useful work that should be re-landed as properly scoped, reviewed PRs:
verbatimModuleSyntax+ type import fixes (standalone)moduleResolution: "bundler"+ dts sync tooling (standalone)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes