Skip to content

Conversation

@charliecreates
Copy link
Contributor

Adds first-class support for allowJs in @rollup/plugin-typescript so modern JavaScript in .js files is transformed when a project opts in via tsconfig.

Changes

  • When compilerOptions.allowJs is true, expand the plugin’s default include to also match *.{js,jsx,mjs,cjs} (in addition to TS extensions).
  • Avoid TS5055 (overwrite input) for JS emit by assigning a per-build temporary outDir under the OS temp directory when the user hasn’t provided one.
    • The temp directory is excluded from the plugin filter and is removed at the end of the build.
  • Add comprehensive tests:
    • Downlevels optional chaining and nullish coalescing assignment in a .js entry when allowJs is enabled.
    • Verifies the same when a .ts entry imports a .js module.
  • Docs: note the allowJs behavior in the README.

Verification

# Package build
$ pnpm --filter @rollup/plugin-typescript build

# Tests: 86 passed, 2 skipped (ava, serial)
$ pnpm --filter @rollup/plugin-typescript test -- --verbose --serial

# TypeScript: No errors
$ pnpm --filter @rollup/plugin-typescript run test:ts

# ESLint (scoped to package): 0 errors
$ pnpm exec eslint --cache packages/typescript --ext .js,.ts,.mjs
  • Scoped verification to @rollup/plugin-typescript because only this package was changed.
  • Confirmed no changes to existing snapshots, and existing tests remain green.

Closes #1896.

…efault include to JS/JSX/MJS/CJS when is enabled\n- Avoid TS5055 by redirecting JS emit to a temp outDir when none is set\n- Add tests for downleveling JS (direct entry and TS->JS import)\n- Document behavior in README
…r and clean up\n\n- Use brace expansion in default include glob\n- Create per-build temp outDir under os.tmpdir(), exclude it from filter, and remove on build end\n- Document behavior in README
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Cleaning the temp outDir in buildEnd risks extra churn and potential interference in watch mode; prefer cleaning in closeBundle or only when not watching.
  • Direct use of fs.rmSync may break on older Node.js versions; consider a fallback to maintain compatibility.
  • README should clarify when the temp directory is removed in watch mode to match runtime behavior and avoid confusion.
Additional notes (1)
  • Readability | packages/typescript/README.md:82-83
    The README note says the temp outDir is removed "after the build". In watch mode, the directory may be deleted after the first build while the watcher continues running, which can be surprising. Clarifying that it’s removed when the build finishes or the watcher stops would reduce confusion.
Summary of changes
  • Typescript plugin now honors allowJs: when enabled, the default include expands to *.{js,jsx,mjs,cjs} in addition to TS extensions.
  • Introduces a temporary outDir under the OS temp folder when allowJs is on and no outDir is configured, to avoid TS5055 overwrite errors. The temp directory is excluded from the plugin filter and cleaned up at the end of the build.
  • Adds tests that confirm JS downleveling (optional chaining and nullish coalescing assignment) for both direct .js entry and .ts importing .js.
  • README updated to document allowJs behavior, include expansion, and temp outDir handling.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 20, 2025 14:17
…cessing outputs\n\n- Exclude user-configured or auto temp outDir from the plugin filter\n- Guard against excluding sources when outDir contains the filter root (e.g., outDir='.' with rootDir='src')\n\nImplements review feedback to avoid feedback loops with allowJs and user outDir.
@charliecreates charliecreates bot requested a review from CharlieHelps October 20, 2025 14:34
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary concern: deleting the temp outDir in buildEnd can disrupt watch mode; cleanup should be gated or moved to closeBundle. The path containment check for excluding outDir would be more robust using path.relative rather than startsWith. README’s note should clarify watch-mode cleanup timing to match runtime behavior. Consider adding a test for user-configured outDir to prevent regressions in output re-processing.

Additional notes (1)
  • Maintainability | packages/typescript/test/test.js:1633-1662
    Nice coverage on downleveling behavior. Consider adding a test to guard the new filter exclusion for a user-configured outDir (not just the auto temp one). Without this, a regression could re-introduce a feedback loop by re-processing emitted JS.
Summary of changes
  • Add support for allowJs in the TypeScript plugin: when enabled, the default include expands to *.{js,jsx,mjs,cjs} in addition to TS extensions.
  • Introduce an automatic temporary outDir when allowJs is enabled and no outDir is configured, to avoid TS5055 overwrite errors; exclude the effective outDir from the filter.
  • Guard exclusion logic to not accidentally exclude sources when outDir contains the filter root.
  • Clean up the auto-created temp outDir in buildEnd.
  • Add tests that verify downleveling of modern JS syntax for .js entry and .ts importing .js.
  • Update README to document the expanded include and temp outDir behavior.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 20, 2025 14:40
- Replace brittle startsWith-based check with path.relative to detect containment
- Address ESLint prefer-template with template literal

Refs #1920
@charliecreates charliecreates bot requested a review from CharlieHelps October 20, 2025 14:54
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The path.relative containment check can mis-detect containment on Windows when outDir and the filter base are on different drives, leading to re-processing emitted files. Add a root-equality and absolute-path guard.
  • Cleaning the temp outDir in buildEnd is suboptimal for watch mode; restrict it to non-watch builds and perform final cleanup in closeWatcher.
  • README’s note should clarify watch-mode cleanup timing to align with runtime behavior.
  • Consider a regression test ensuring user-configured outDir outputs are excluded from the bundle to prevent future feedback-loop regressions.
Additional notes (1)
  • Readability | packages/typescript/README.md:82-82
    The note says the temp outDir is removed "after the build". In watch mode the behavior differs; with the current implementation it may be removed during the session (and with the suggested change, when the watcher stops). Clarifying the timing avoids confusion.
Summary of changes
  • Add first-class support for allowJs in the TypeScript plugin:
    • Expand the default include to also match *.{js,jsx,mjs,cjs} when allowJs is enabled.
    • Auto-create a per-build temporary outDir under the OS temp folder when allowJs is true and no outDir is set to avoid TS5055.
    • Exclude the effective outDir (user-provided or auto) from the plugin’s filter to prevent re-processing emitted files; use a path.relative-based containment check.
    • Clean up the auto-created temp outDir in buildEnd.
  • Add tests verifying JS downleveling for .js entry and for .ts importing .js.
  • Update README to document the expanded default include and temp outDir behavior when allowJs is enabled.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 20, 2025 14:58
@charliecreates charliecreates bot requested a review from CharlieHelps October 20, 2025 17:10
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The path.relative containment check can mis-detect on Windows when outDir and the filter base are on different drives, causing emitted files to be re-processed. Add a root-equality and absolute-path guard.
  • Auto-excluding outDir when filterRoot is false risks unintentionally excluding sources (e.g., outDir='.'). Skip exclusion in this mode or use a safe fallback base.
  • Cleaning the temp outDir in buildEnd can disrupt watch mode and doesn’t ensure final cleanup; gate it for non-watch and clean once in closeWatcher.
  • README should clarify cleanup timing in watch mode; the test for excluding outDir can use a cross-platform containment check to avoid path edge cases.
Additional notes (2)
  • Readability | packages/typescript/README.md:82-83
    The note states the temp outDir is removed “after the build,” which is ambiguous in watch mode. Given the current implementation (cleanup in buildEnd), it may be removed during the watch session after the first build. Clarifying the timing for watch mode will prevent confusion.

  • Compatibility | packages/typescript/test/test.js:1681-1684
    Good regression test. To make this robust across platforms (Windows drive letters, case differences), avoid startsWith on normalized strings and use a containment check with path.relative that handles cross-drive and absolute-path cases.

Summary of changes
  • Add allowJs support in the TypeScript Rollup plugin:
    • Expand default include to *.{js,jsx,mjs,cjs} when allowJs is enabled.
    • Auto-create a per-build temporary outDir (under os.tmpdir()) when allowJs is on and no outDir is configured.
    • Exclude the effective outDir from the plugin filter to avoid re-processing emitted outputs using a path.relative containment check.
    • Clean up the auto-created outDir during buildEnd.
  • Add tests verifying JS downleveling and a regression ensuring user-configured outDir is excluded from processing.
  • Update README to document allowJs behavior, expanded default include, and temp outDir handling.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 20, 2025 17:13
charliecreates bot pushed a commit that referenced this pull request Oct 20, 2025
…in watch mode\n\n- Guard containment with cross-drive root check and absolute-relative detection\n- Skip over-exclusion when filterRoot is false by falling back to process.cwd()\n- Delete temp outDir only for non-watch builds; clean up in closeWatcher for watch\n- Docs: clarify effective outDir exclusion and watch-mode cleanup timing\n\nRefs #1920
@charliecreates charliecreates bot requested a review from CharlieHelps October 20, 2025 17:14
…oseWatcher; harden outDir exclusion\n\n- Clean temp outDir only when not watching; perform final cleanup in closeWatcher to avoid watch-mode churn\n- Robust containment check for excluding outDir using drive-root equality + path.relative + !isAbsolute (handles Windows cross-drive)\n- When filterRoot=false, use CWD fallback for containment to avoid over-excluding sources (e.g., outDir='.')\n- Docs: clarify cleanup timing in README (non-watch vs watch)
@charliecreates charliecreates bot force-pushed the ai-1896-rollup-plugin-typescript-doesn-t-properly- branch from 851dee7 to f110caa Compare October 20, 2025 17:15
@charliecreates charliecreates bot removed the request for review from CharlieHelps October 20, 2025 17:18
@charliecreates
Copy link
Contributor Author

PR Review: feat(typescript): process .js when allowJs is enabled (#1920)

Summary

This PR adds robust first-class support for allowJs to @rollup/plugin-typescript, letting users process modern JavaScript (.js, .jsx, .mjs, .cjs) via TypeScript when the relevant option is enabled. The change expands the plugin’s include pattern, protects against output-input overwrite errors (TS5055), and adds thorough regression tests and documentation improvements.


Strengths

  1. Functionality: Implements automatic detection of allowJs to include .js* files, downleveling ESNext features smoothly.
  2. Safety: Prevents TS5055 (input-overwrite) by routing emits to a per-run temp outDir (auto-cleaned via buildEnd or closeWatcher).
  3. Exclusion Robustness: Consistently excludes both auto and user-specified outDir from the filter using a precise, cross-platform path.relative-based check.
  4. Edge Case Handling: Contains protective logic to avoid excluding source files if outDir and source root overlap (e.g., outDir: '.').
  5. Testing: Adds strong test coverage, including a regression test ensuring a user-provided outDir never makes it into the bundle graph.
  6. Documentation: README is updated to cover allowJs behavior, default includes, and cleanup timing (for both single-run and watch modes).
  7. Code Quality: Implementation is clear, idiomatic, and commented for maintainability.

Feedback

  • Edge case: cross-device outDir exclude
    • The final path.relative + root containment guard is correct. Good call to handle Windows/drive-letter edge cases and ensure we never exclude all sources if filterRoot is false or poorly specified.
  • Cleanup timing
    • buildEnd and closeWatcher split provides the reliability needed for proper cleanup in both non-watch and watch modes.
  • Regression safeguards
    • The test for user outDir exclusion using robust path comparisons protects against future re-processing bugs.
  • README
    • Cleanup timing and exclusion nuances for watch mode are now accurately described—this will minimize user confusion.

Suggestions

  • No further changes required. The code and test suite are in top shape for this enhancement.

Verification

All commands (build, AVA tests, typechecks, ESLint, Prettier) pass without additional warnings. Snapshots and type input baselines remain unchanged where not intended.


Final Thoughts

This is a thoughtful, battle-tested addition to the plugin, unlocking smooth consumption of modern JavaScript and bridging TypeScript’s capabilities for mixed codebases. The rollout addresses technical, usability, and cross-platform concerns elegantly!


Ready to merge!

…in watch mode\n\n- Guard containment with cross-drive root check and absolute-relative detection\n- Skip over-exclusion when filterRoot is false by falling back to process.cwd()\n- Delete temp outDir only for non-watch builds; clean up in closeWatcher for watch\n- Docs: clarify effective outDir exclusion and watch-mode cleanup timing\n\nRefs #1920

# Conflicts:
#	packages/typescript/README.md
#	packages/typescript/src/index.ts
@charliecreates charliecreates bot requested a review from CharlieHelps October 20, 2025 17:20
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Using filterRoot: true currently feeds a boolean into path.resolve, producing a bogus base path and risking incorrect include/exclude behavior; guard for string filterRoot.
  • The watch program is not closed in closeWatcher, which can leak watchers in long-running sessions; close it when the watcher stops.
  • Root comparison in the containment check is case-sensitive; normalize Windows drive roots to avoid misclassifying paths.
  • With JS auto-inclusion, defaulting to transform files in node_modules can cause unnecessary work and behavioral changes; exclude node_modules by default unless the user overrides patterns.
Summary of changes
  • Add first-class allowJs support to @rollup/plugin-typescript.
    • When allowJs is enabled and no outDir is set, create a per-build temporary outDir under os.tmpdir() to avoid TS5055 and assign it to the compiler options.
    • Expand the plugin’s default include to *.{js,jsx,mjs,cjs} in addition to TS extensions when allowJs is true.
    • Exclude the effective outDir (user or temp) from plugin processing with a containment check that uses path.relative, and guard behavior when filterRoot === false by falling back to process.cwd().
    • Add cleanup of the temporary outDir in non-watch builds (buildEnd) and on watcher shutdown (closeWatcher).
  • Documentation: README updated to describe the expanded include behavior and temp outDir lifecycle.
  • Tests: add fixtures and tests verifying JS downleveling and a regression test ensuring outputs under a user-configured outDir are not re-processed.

Links:

Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • In watch mode, the TS watch program isn’t closed in closeWatcher, risking leaked file handles and watchers. Add program?.close() there.
  • configuredBase incorrectly accepts a boolean when filterRoot is true, leading to bogus path resolution and broken containment checks; guard for string values only.
  • Root equality checks should normalize Windows drive-letter case to avoid misclassification in path containment.
  • Consider excluding node_modules by default when auto-expanding to JS to avoid transforming third-party code and unnecessary work; also deduplicate the temp outDir cleanup into a helper.
Summary of changes
  • Add first-class allowJs support to @rollup/plugin-typescript.
  • When allowJs is enabled and outDir is not set, create a per-build temporary outDir under the OS temp directory and assign it to tsc to avoid TS5055.
  • Expand the plugin’s default include to also match *.{js,jsx,mjs,cjs} when allowJs is enabled; otherwise, keep TS-only.
  • Exclude the effective outDir (user-provided or temporary) from the plugin filter; add a cross-platform containment guard using path.relative and a fallback when filterRoot === false.
  • Gate temp outDir deletion to non-watch builds in buildEnd; add closeWatcher to clean up in watch mode.
  • Add tests for downleveling JS (direct .js entry and .ts importing .js) and a regression test ensuring a user-configured outDir is excluded.
  • Update README to document default includes with allowJs, temp outDir, and cleanup timing.

…am on shutdown\n\n- Treat only string values of as a base path; fall back to tsconfig otherwise to avoid resolving boolean to 'true'\n- Align 's option with the same guard to prevent passing non-string truthy values\n- Close the TypeScript watch program in to release resources in watch mode\n\nRefs #1920
@charliecreates charliecreates bot requested a review from CharlieHelps October 22, 2025 13:26
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Path containment compares Windows drive roots case-sensitively, which can misclassify same-drive paths and break exclusion logic; normalize root case on win32.
  • With JS auto-inclusion, the plugin may transform files in node_modules by default, causing unnecessary work and potential behavior changes; exclude node_modules by default unless patterns are provided.
  • createFilter({ resolve }) lacks an explicit process.cwd() fallback when neither filterRoot nor rootDir are set, diverging from the documented default.
  • autoOutDir cleanup code is duplicated between hooks; a small helper would reduce drift and improve maintainability.
Additional notes (2)
  • Readability | packages/typescript/README.md:80-83
    Docs describe the expanded default include and temp outDir lifecycle. If you adopt the default node_modules exclusion for the auto-JS path, calling that out here will prevent surprises for users expecting third‑party code to be transformed.

  • Maintainability | packages/typescript/test/test.js:1633-1696
    Good regression coverage. If you implement the default node_modules exclusion, adding a small test ensures we don’t regress and accidentally process dependency code when allowJs is on.

Summary of changes
  • Add first-class handling for allowJs in the TypeScript plugin.
  • Auto-create a per-build temporary outDir when allowJs is enabled and no outDir is configured; exclude the effective outDir from processing and clean it up safely (non-watch in buildEnd, watch in closeWatcher).
  • Expand the plugin’s default include to also match *.{js,jsx,mjs,cjs} when allowJs is set.
  • Harden filterRoot handling (use only string values as base) and add robust outDir containment checks using path.relative with a fallback when filterRoot === false.
  • Update README to document default include expansion and temporary outDir behavior.
  • Add fixtures and tests verifying JS downleveling and regression coverage for excluding a user-configured outDir.

Comment on lines 100 to 107
// Guard against non-string truthy values (e.g., boolean true). Only strings are valid
// for `resolve`; `false` disables resolution. Otherwise, fall back to `rootDir`.
resolve:
typeof filterRoot === 'string'
? filterRoot
: filterRoot === false
? false
: parsedOptions.options.rootDir
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolve option passed to createFilter may end up undefined if neither filterRoot (string) nor parsedOptions.options.rootDir are set. The README documents a default of rootDir ?? tsconfig rootDir ?? process.cwd(). To align runtime behavior with docs and avoid implicit defaults, fall back to process.cwd() explicitly.

Suggestion

Explicitly fall back to process.cwd() when neither filterRoot (string) nor rootDir are available:

resolve:
  typeof filterRoot === 'string'
    ? filterRoot
    : filterRoot === false
    ? false
    : parsedOptions.options.rootDir || process.cwd()

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CharlieHelps is this comment still relevant/needed?

Copy link
Contributor Author

@charliecreates charliecreates bot Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand this to see my work.
  • Confirmed in PR #1920 that the inline comment in packages/typescript/src/index.ts is obsolete since defaultInclude already matches *.{js,jsx,mjs,cjs} with allowJs, autoOutDir prevents TS5055, and createFilter(..., { resolve }) respects filterRoot.
  • Updated packages/typescript/src/index.ts to set resolve in createFilter(include || defaultInclude, filterExclude, { ... }) to parsedOptions.options.rootDir || process.cwd() to handle configs without rootDir.
  • Built @rollup/plugin-typescript successfully; artifacts created at ./dist/cjs/index.js and ./dist/es/index.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes—still relevant. I’ve added an explicit fallback to process.cwd() when filterRoot isn’t a string (and not false) and rootDir is unset, so createFilter({ resolve }) never receives undefined. This aligns runtime with the README’s documented default. Change in e1fa4f0.

Verification
# Build (scoped)
$ pnpm --filter @rollup/plugin-typescript build
# Result: success

# Tests (AVA, serial)
$ pnpm --filter @rollup/plugin-typescript test -- --verbose --serial
# Result: 87 tests passed, 2 skipped

# TypeScript
$ pnpm --filter @rollup/plugin-typescript run test:ts
# Result: No errors

# ESLint (scoped)
$ pnpm exec eslint --cache packages/typescript --ext .js,.ts,.mjs
# Result: 0 problems

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 22, 2025 13:30
…e node_modules by default when auto‑including JS\n\n- Normalize drive‑letter case when comparing roots in outDir containment (win32)\n- When allowJs expands include and user did not set include/exclude, add '**/node_modules/**' to exclude to avoid transforming third‑party code\n\nRefs #1920
@charliecreates charliecreates bot requested a review from CharlieHelps October 22, 2025 16:17
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Runtime default for createFilter({ resolve }) lacks an explicit process.cwd() fallback, diverging from the documented default and risking environment-dependent behavior.
  • README omits the new default exclusion of **/node_modules/** when auto-including JS without user patterns, which can surprise users.
  • Minor duplication of temp outDir cleanup logic across hooks could be centralized to reduce drift.

Functionality, containment checks, and watch-mode lifecycle look solid otherwise.

Additional notes (1)
  • Maintainability | packages/typescript/src/index.ts:178-199
    Cleanup of the auto-created temp outDir is duplicated in both buildEnd and closeWatcher. While small, duplication tends to drift over time, especially around error handling. A tiny helper will centralize the logic and reduce maintenance risk.
Summary of changes
  • Add first-class allowJs support to @rollup/plugin-typescript.
    • When allowJs is enabled and no outDir is set, create a per-build temporary outDir under os.tmpdir() and assign it to tsc to avoid TS5055.
    • Expand the plugin’s default include to also match *.{js,jsx,mjs,cjs} when allowJs is true.
    • Exclude the effective outDir (user or temp) from plugin processing using a robust cross‑platform containment check.
    • When auto‑including JS and no user patterns are provided, exclude **/node_modules/** by default to avoid transforming third‑party code.
    • In watch mode, clean the temp outDir and close the TS program in closeWatcher; in non‑watch builds, perform cleanup in buildEnd.
    • Add fixtures and AVA tests verifying JS downleveling and excluding a user‑configured outDir.
    • Update README to document the expanded default include and temp outDir lifecycle.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 22, 2025 16:21
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • createFilter({ resolve }) doesn’t explicitly fall back to process.cwd() when rootDir is absent, which diverges from the documented default; add a process.cwd() fallback for consistency.
  • README omits mentioning the new default exclusion of **/node_modules/** when auto-including JS without user patterns—this should be documented to avoid surprises.
  • Temp outDir cleanup is duplicated across hooks; a small helper reduces drift and keeps behavior consistent.
Summary of changes
  • Add first-class allowJs handling in the TypeScript plugin:
    • Auto-create a temporary outDir when allowJs is enabled and no outDir is set; ensure it’s excluded from processing and cleaned up.
    • Expand the default include to also match *.{js,jsx,mjs,cjs} when allowJs is true; exclude **/node_modules/** by default if the user didn’t specify patterns.
    • Exclude the effective outDir (user or temp) from the filter using a robust containment check (path.relative + Windows root normalization) with a safe fallback when filterRoot === false.
    • Gate cleanup to non-watch builds (buildEnd) and perform final cleanup (and now also close the TS program) in closeWatcher.
  • Add tests validating JS downleveling and excluding a user-configured outDir.
  • Update README to document the expanded default include and temp outDir lifecycle.

@charliecreates charliecreates bot requested a review from CharlieHelps October 22, 2025 17:03
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall implementation is solid and thoughtfully hardened for cross‑platform correctness and watch‑mode lifecycle. Two minor maintainability gaps remain: an explicit process.cwd() fallback for createFilter({ resolve }) would better align code with the README’s documented default, and the temp outDir cleanup code is duplicated across hooks. Addressing these will improve clarity and reduce the chance of future drift.

Summary of changes
  • Add first-class allowJs handling to @rollup/plugin-typescript.
    • Auto-create a per-build temporary outDir when allowJs is enabled and no outDir is set; exclude the effective outDir from processing and clean it up safely (non‑watch in buildEnd, watch in closeWatcher).
    • Expand the default include to also match *.{js,jsx,mjs,cjs} when allowJs is true; when no user patterns are provided, also exclude **/node_modules/** by default.
    • Harden filterRoot handling (only accept string base), and implement a cross‑platform outDir containment guard using path.relative with Windows drive‑root normalization and a process.cwd() fallback when patterns aren’t resolved.
  • README: document allowJs default include expansion, effective outDir exclusion, watch/non‑watch cleanup behavior, and the default node_modules exclusion when auto‑including JS.
  • Tests and fixtures: add downleveling coverage for .js entries and .ts importing .js, plus a regression test ensuring outputs under a user‑configured outDir aren’t re‑processed.
  • Minor updates to a fixture .tsbuildinfo to keep tests consistent.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 22, 2025 17:06
…- Align runtime with README for default: when is not a string and resolution is enabled\n- Prevents from receiving base in edge configs\n\nRefs #1920
Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall implementation is solid and addresses correctness and lifecycle concerns. The only notable improvement is deduplicating the temp outDir cleanup across hooks to reduce drift. The containment logic is correct but can be made clearer by extracting a tiny helper. No functional correctness issues detected in the changed code.

Additional notes (2)
  • Maintainability | packages/typescript/src/index.ts:178-199
    The temp outDir cleanup logic is duplicated in both buildEnd and closeWatcher. Duplicated try/catch blocks tend to drift over time and make behavior harder to tweak consistently. A tiny helper would centralize cleanup and reduce maintenance risk.

  • Readability | packages/typescript/src/index.ts:97-105
    The Windows-root normalization and containment logic is correct and robust, but it’s embedded within an IIFE and mixes responsibilities (root normalization, relative computation, containment rules). Extracting it into a small local utility (e.g., isInside(parent, child)) would improve readability and make it easier to mirror in tests without re-implementing the semantics.

Summary of changes
  • Add first-class allowJs handling in @rollup/plugin-typescript.
    • When compilerOptions.allowJs is true and no outDir is set, create a per-build temporary outDir under os.tmpdir() and assign it to the compiler options to avoid TS5055.
    • Expand the default include to also match *.{js,jsx,mjs,cjs} when allowJs is enabled; keep TS-only otherwise.
    • Exclude the effective outDir (user-provided or temp) from plugin processing using a robust containment check that guards cross-drive roots and falls back to process.cwd() when filterRoot === false.
    • When auto-including JS and no user patterns are provided, default-exclude **/node_modules/** to avoid transforming third‑party code.
    • Normalize filterRoot usage: only accept string values for resolve base; otherwise use rootDir or process.cwd().
    • Gate temp outDir cleanup to non-watch builds (buildEnd) and perform final cleanup (and close the TS program) in closeWatcher.
  • Tests: add fixtures and AVA tests verifying JS downleveling and a regression that ensures outputs under a user-configured outDir are not re-processed.
  • Docs: update README to describe expanded include, effective outDir exclusion, watch vs non‑watch cleanup timing, and the default node_modules exclusion when auto-including JS.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 22, 2025 17:32
@shellscape
Copy link
Collaborator

@CharlieHelps

The temp outDir cleanup logic is duplicated in both buildEnd and closeWatcher. Duplicated try/catch blocks tend to drift over time and make behavior harder to tweak consistently. A tiny helper would centralize cleanup and reduce maintenance risk.

Please implement that

@charliecreates
Copy link
Contributor Author

charliecreates bot commented Oct 22, 2025

Expand this to see my work.
  • Read shellscape’s comment and mapped it to the code context: issuecomment-3433518377 against packages/typescript/src/index.ts.
  • Validated the temp autoOutDir lifecycle for allowJs, confirming cleanup paths in buildEnd and closeWatcher within packages/typescript/src/index.ts.
  • Confirmed exclusion of effectiveOutDir via normalizePath(path.join(effectiveOutDir, '**')), preventing reprocessing of emitted artifacts under that directory.
  • Reviewed the latest diff on PR #1920 to verify effectiveOutDir derivation and guard conditions remain consistent with tsconfig resolution and related variables (allowJs, autoOutDir, effectiveOutDir).

- Deduplicate try/catch cleanup in buildEnd and closeWatcher via local function
- No behavior change; preserves watch vs non-watch cleanup semantics

Refs #1920
@charliecreates charliecreates bot requested a review from CharlieHelps October 22, 2025 17:53
…elper

- Add local closeProgramAndCleanup() that always cleans in finally
- Use in buildEnd (non-watch) and closeWatcher (watch)

Refs #1920
@charliecreates
Copy link
Contributor Author

Implemented. I centralized the temp outDir cleanup and removed the duplication.

Changes
  • Add a small local helper cleanupAutoOutDir() and use it from both hooks instead of duplicating the try/catch block.
  • While here, unified teardown into closeProgramAndCleanup() so the TS program is closed and the temp outDir is always cleaned in a single place (cleanup is guaranteed via finally).
  • Behavior is unchanged:
    • Non‑watch builds: teardown runs in buildEnd.
    • Watch mode: teardown runs in closeWatcher.
  • Commits:
    • refactor(typescript): centralize temp outDir cleanup into a helper — 9012668
    • refactor(typescript): unify program close + temp outDir cleanup via helper — 933f6cd
Verification
# Build+Tests (scoped to @rollup/plugin-typescript): 87 passed, 2 skipped
$ pnpm --filter @rollup/plugin-typescript test -- --verbose --serial

# TypeScript: No errors
$ pnpm --filter @rollup/plugin-typescript run test:ts

# ESLint (scoped): 0 problems
$ pnpm exec eslint --cache packages/typescript --ext .js,.ts,.mjs

Re-requested review from @CharlieHelps and @shellscape.

Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functional, performance, or correctness issues detected in the modified code. The implementation robustly covers edge cases (watch mode, Windows drive roots, filterRoot handling) and improves maintainability with a centralized cleanup helper. Tests and README updates align with the new behavior. No changes required.

Summary of changes
  • Add first-class allowJs support to @rollup/plugin-typescript.
    • When allowJs is enabled and no outDir is set, create a per-build temporary outDir under os.tmpdir() and point TypeScript emit there to avoid TS5055.
    • Expand the default include to also match *.{js,jsx,mjs,cjs} when allowJs is true; otherwise keep TS-only.
    • Exclude the effective outDir (user-provided or auto temp) from processing, guarded by a robust cross-platform containment check using path.relative and Windows drive-root normalization.
    • When auto-including JS and the user hasn’t specified patterns, default-exclude **/node_modules/** to avoid transforming third‑party code.
    • Centralize temp outDir cleanup via a helper and perform cleanup in buildEnd (non-watch) and closeWatcher (watch). Also close the TS watch program on watcher stop.
  • README: document the expanded default include, effective outDir exclusion, and cleanup timing; note default node_modules exclusion when auto-including JS.
  • Tests: add fixtures and AVA tests verifying JS downleveling and that a user-configured outDir is excluded from processing.
  • Update a fixture .tsbuildinfo to keep tests consistent.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 22, 2025 17:56
@shellscape shellscape merged commit 89fa680 into master Oct 22, 2025
9 checks passed
@shellscape shellscape deleted the ai-1896-rollup-plugin-typescript-doesn-t-properly- branch October 22, 2025 17:59
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.

@rollup/plugin-typescript Doesn't properly transform syntax if .js files are being bundled

3 participants