Skip to content

Conversation

xswei
Copy link

@xswei xswei commented Oct 11, 2021

Fixed the bug that the prototype was lost

See #34

@codecov-commenter
Copy link

Codecov Report

Merging #36 (60fd92f) into main (efe07a8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@ ## main #36 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 2 2 Lines 86 86 Branches 13 13 ========================================= Hits 86 86 
Impacted Files Coverage Δ
src/arrayToTree.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efe07a8...60fd92f. Read the comment docs.

@philipstanislaus
Copy link
Owner

Thanks 🙏

For some context why this change is made: https://2ality.com/2016/10/rest-spread-properties.html#spreading-objects-versus-object.assign()

TLDR: Object.assign in the way used here calls setters on the item and allows setters to be called.

I still need to think about any issues with this change such that users may perceive this as a breaking change - do you have any thoughts on that?

@xswei
Copy link
Author

xswei commented Oct 12, 2021

Maybe you can let the user decide which way to choose, such as configuration: clone: 'spread' | 'assign'

@philipstanislaus
Copy link
Owner

Sorry for the delay on this.

I like the idea of giving an option to the user – implemented that change in #44 .

Released in https://www.npmjs.com/package/performant-array-to-tree/v/1.11.0

Thanks for your contribution!

Closes #34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants