Conversation
🦋 Changeset detectedLatest commit: 271adfa The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes update Node.js engine requirements to version 18 across multiple package manifests, remove the "tslib" dependency, and adjust dependency versions for consistency. The TypeScript configuration now explicitly targets ES2022. In addition, the build process is refactored by centralizing the CLI invocation through a new script in the root package.json and updating build command options. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Yarn as Yarn CLI
participant TSX as TSX Runtime
participant CLI as CLI Script
Dev->>Yarn: yarn build:r
Yarn->>Yarn: Run "r" script with flags and esbuild option
Yarn->>TSX: tsx packages/rollup/src/cli.ts -f cjs -d false -e named --esbuild '{target:es2022}'
TSX->>CLI: Execute CLI logic
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR bumps the Node version to 18 and updates the TypeScript target to ES2022, while removing the dependency on tslib. Key changes include:
- Updating tsconfig.base.json to extend the Node 16 config with additional compilerOptions for ES2022.
- Bumping the Node engine requirement to >=18 and removing tslib from several package.json files.
- Modifying the build scripts in package.json to leverage the updated Node version and ES2022 target.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.base.json | Updated to configure ES2022 target for TypeScript. |
| packages/rollup/package.json | Updated Node engine requirement and removed tslib dependency. |
| packages/imagemin/package.json | Updated Node engine requirement and removed tslib dependency. |
| packages/browser/package.json | Updated Node engine requirement and adjusted dependencies to remove tslib. |
| package.json | Updated build scripts to align with the Node and ES2022 changes; added new "r" script. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9a4c03b in 1 minute and 28 seconds. Click for details.
- Reviewed
103lines of code in5files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:17
- Draft comment:
Verify that the --esbuild argument '--esbuild '{target:es2022}'' is correctly parsed on all platforms; consider using JSON-quoted syntax if issues arise. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the parsing of an argument across all platforms, which falls under the rule of not asking the author to double-check or ensure behavior. It also suggests using JSON-quoted syntax if issues arise, which is a suggestion but is conditional on verification, making it less direct. This comment is not approved as it violates the rules.
2. packages/imagemin/package.json:41
- Draft comment:
The tslib dependency has been removed. Confirm that no parts of the code rely on tslib helpers and that native alternatives are in place. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that no parts of the code rely on tslib helpers after its removal. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or to double-check things.
3. packages/rollup/package.json:50
- Draft comment:
Removal of tslib dependency in rollup package; ensure that any helper functions previously provided by tslib are now handled correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is related to a dependency change, specifically the removal of the tslib dependency. The comment asks the author to ensure that helper functions previously provided by tslib are handled correctly. This falls under the rule of not commenting on dependency changes or asking the author to ensure things are handled correctly.
4. tsconfig.base.json:2
- Draft comment:
While targeting ES2022, the config still extends '@1stg/tsconfig/node16'. Confirm there are no conflicting compiler options; consider updating to a Node18-specific config if available. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment suggests changing to node18 config, but this is speculative - we don't know if there are actual conflicts. The author included documentation link showing they deliberately chose ES2022 target. This feels like asking the author to double-check their intention, which violates our rules. The comment also uses "confirm" language which is explicitly discouraged. The comment might be identifying a real technical issue with TypeScript configuration that could cause problems. Node version and ES target mismatches can cause runtime issues. While configuration mismatches can be problematic, the author shows awareness of version mapping through the included comment link. Without concrete evidence of an actual conflict, this is just speculation and asking for confirmation. Delete the comment as it's speculative and asks for confirmation rather than pointing out a definite issue. The author shows awareness of version mapping in their comment.
Workflow ID: wflow_yCvruP5gdFI1a1Uj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tsconfig.base.json (1)
2-3: Consider using a Node18 base config
You’re extending@1stg/tsconfig/node16but targeting ES2022. If anode18preset exists, switching to it could avoid the manual override and keep configs aligned.package.json (1)
17-17: Avoid Windows quoting issues
Single quotes around{target:es2022}may not work on Windows shells.Apply this diff to ensure cross-platform compatibility:
- "build:r": "yarn r -f cjs -d false -e named --esbuild '{target:es2022}'", + "build:r": "yarn r -f cjs -d false -e named --esbuild \"{target:es2022}\"",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)packages/browser/package.json(2 hunks)packages/imagemin/package.json(2 hunks)packages/rollup/package.json(1 hunks)tsconfig.base.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tsconfig.base.json
[error] 4-4: Expected a property but instead found '// https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-18'.
Expected a property here.
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
packages/imagemin/package.json (2)
12-12: Bump Node engine to 18
Aligns with the repo-wide requirement for Node.js 18+.
44-44: Verify tinyglobby retention vs summary
The AI summary indicated thattinyglobbywas removed alongsidetslib, yet it still appears here. Please confirm whether this dependency should indeed be retained or if it was meant to be dropped.Likely an incorrect or invalid review comment.
packages/rollup/package.json (1)
12-12: Bump Node engine to 18
Consistent with other packages now targeting Node.js 18+.packages/browser/package.json (3)
12-12: Bump Node engine to 18
Aligns this package with the monorepo’s minimum Node.js 18 requirement.
34-34: Pinopendependency version
Restrictsopento^10.1.2, ensuring a known stable release.
35-35: Simplifypicocolorsversion
Updated to^1.1.1, removing unnecessary precision.tsconfig.base.json (1)
2-6: Override TS target to ES2022
Explicitly setstargettoES2022to leverage Node.js 18 ECMAScript features. JSONC supports inline comments, so the URL comment is expected.🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Expected a property but instead found '// https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-18'.
Expected a property here.
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
package.json (1)
27-27: Addrscript to invoke Rollup CLI
Introduces a consistent entry point for running the Rollup CLI viatsx.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
@pkgr/browser
@pkgr/core
@pkgr/es-modules
@pkgr/imagemin
@pkgr/rollup
@pkgr/umd-globals
@pkgr/utils
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #406 +/- ##
======================================
Coverage 9.85% 9.85%
======================================
Files 12 12
Lines 426 426
Branches 186 189 +3
======================================
Hits 42 42
Misses 384 384 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Deploy preview for pkgr ready! ✅ Preview Built with commit 271adfa. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 271adfa in 1 minute and 36 seconds. Click for details.
- Reviewed
116lines of code in6files - Skipped
1files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/short-pets-beam.md:1
- Draft comment:
Changeset file format looks good; ensure the version bumps are intentional. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. package.json:17
- Draft comment:
Review the '--esbuild' flag value; single quotes around {target:es2022} might be misinterpreted in some shells. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Shell interpretation of quotes can be tricky, but this is in package.json which is parsed as JSON, not directly by a shell. The actual shell interpretation would depend on how yarn and the underlying tools handle the arguments. Without strong evidence that this causes actual problems, this seems speculative. The current format may actually be safer in some shells. I could be wrong about how yarn handles script arguments. There might be specific shell environments where this is a real issue. Without concrete evidence of a problem or documentation showing the correct format, this comment is speculative and could potentially make things worse. The comment should be deleted as it makes a speculative suggestion about shell behavior without clear evidence of an actual problem.
3. packages/browser/package.json:12
- Draft comment:
Node engine updated to '>=18'; ensure documentation and CI are updated to reflect this breaking change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/imagemin/package.json:12
- Draft comment:
Node engine bumped to '>=18' and tslib removed; confirm that all image-minification workflows still work as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/rollup/package.json:12
- Draft comment:
Updated Node engine to '>=18'; ensure this breaking change is well documented for users. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/rollup/package.json:50
- Draft comment:
Removal of tslib dependency appears intentional; verify that no helper functions are missing post-removal. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. tsconfig.base.json:1
- Draft comment:
Now targeting ES2022 while still extending a Node16 config; consider switching to a Node18-specific config if available. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_U4tPimgl5xTKAIjT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tsconfig.base.json (2)
4-6: Align TypeScript target with Node 18 features.
Setting"target": "ES2022"is exactly what's needed to leverage Node 18 syntax. The inline link is a useful reference.Optionally, if your base config doesn’t already include it, add
"lib": ["ES2022"]undercompilerOptionsto ensure proper type definitions for new built-ins.🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Expected a property but instead found '// https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-18'.
Expected a property here.
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
4-5: Suppress linter errors on JSONC comments.
Biome is flagging the// …comment as invalid JSON. Since tsconfig files support JSONC, either adjust Biome’s parser settings to allow comments in.jsonor rename this file to.jsoncto eliminate false positives.🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Expected a property but instead found '// https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-18'.
Expected a property here.
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 5-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/short-pets-beam.md(1 hunks)package.json(2 hunks)packages/browser/package.json(2 hunks)packages/imagemin/package.json(2 hunks)packages/rollup/package.json(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .changeset/short-pets-beam.md
- packages/rollup/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/imagemin/package.json
- packages/browser/package.json
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
tsconfig.base.json
[error] 4-4: Expected a property but instead found '// https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping#node-18'.
Expected a property here.
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 5-5: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
🔇 Additional comments (1)
tsconfig.base.json (1)
2-2: Verify base configuration alignment.
Theextendsfield still points to@1stg/tsconfig/node16. Since we've bumped to Node 18/ES2022, ensure there's no mismatch—either switch to a Node 18–specific base config (e.g.@1stg/tsconfig/node18) or confirm that your Node 16 config fully covers ES2022 features.
Important
Update Node.js requirement to 18, remove
tslib, and target ES2022 in TypeScript configuration.@pkgr/browser,@pkgr/imagemin, and@pkgr/rollup.tslibfrom@pkgr/browser,@pkgr/imagemin, and@pkgr/rollup.tsconfig.base.jsonto target ES2022.build:rscript in rootpackage.jsonto use ES2022 target.This description was created by
for 271adfa. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit