-
Notifications
You must be signed in to change notification settings - Fork 5
fix: replace object spread with Object.assign in map calls #2405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Co-authored-by: stipsan <[email protected]>
|
|
||
| const exports = Object.entries(ctx.exports || {}).map( | ||
| ([_path, exp]) => ({_path, ...exp}) as PkgExport & {_path: string}, | ||
| ([_path, exp]) => Object.assign({_path}, exp) as PkgExport & {_path: string}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Isn't it better to do Object.assign(exp, {_path})?
| ([_path, exp]) => Object.assign({_path}, exp) as PkgExport & {_path: string}, | |
| ([_path, exp]) => Object.assign(exp, {_path}) as PkgExport & {_path: string}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially used Object.assign(exp, {_path}) as you suggested, but the code review tool flagged a potential issue: mutating the original exp objects from ctx.exports could cause side effects since those objects are also used in check.ts.
Instead, I used Object.assign({}, exp, {_path}) which:
- Satisfies the oxlint rule by using
Object.assign - Creates a new object (avoiding mutation)
- Ensures
_pathis correctly added
See commit b8514b8.
|
|
||
| const exports = Object.entries(ctx.exports || {}).map( | ||
| ([_path, exp]) => ({_path, ...exp}) as PkgExport & {_path: string}, | ||
| ([_path, exp]) => Object.assign({_path}, exp) as PkgExport & {_path: string}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Isn't it better to do Object.assign(exp, {_path})?
| ([_path, exp]) => Object.assign({_path}, exp) as PkgExport & {_path: string}, | |
| ([_path, exp]) => Object.assign(exp, {_path}) as PkgExport & {_path: string}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially used Object.assign(exp, {_path}) as you suggested, but the code review tool flagged a potential issue: mutating the original exp objects from ctx.exports could cause side effects since those objects are also used in check.ts.
Instead, I used Object.assign({}, exp, {_path}) which:
- Satisfies the oxlint rule by using
Object.assign - Creates a new object (avoiding mutation)
- Ensures
_pathis correctly added
See commit b8514b8.
Co-authored-by: stipsan <[email protected]>
Co-authored-by: stipsan <[email protected]>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
oxlint 1.33.0 introduces the
no-map-spreadrule, which flags object spreading in map callbacks as inefficient.Changes
({_path, ...exp})withObject.assign({}, exp, {_path})in:resolveBuildTasks.tsresolveWatchTasks.tsThe implementation uses
Object.assign({}, exp, {_path})to:no-map-spreadrule by usingObject.assigninstead of spreadexpobjects fromctx.exports_pathis correctly added to the result✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.