Skip to content
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

fix(core): fix freezing of source object after mapping #468

Merged
merged 3 commits into from
May 6, 2022

Conversation

LennartH
Copy link
Contributor

Prevent freezing of the given source object as side effect of map(Array)/mutate(Array).
Create shallow copy of source before freezing it and using the frozen copy for the remaining
mapping process.

ISSUES CLOSED: #467

Prevent freezing of the given source object as side effect of map(Array)/mutate(Array).
Create shallow copy of source before freezing it and using the frozen copy for the remaining
mapping process.

ISSUES CLOSED: nartc#467
@nartc
Copy link
Owner

nartc commented May 4, 2022

Let's just remove the freeze completely. I haven't tested it but I think it will affect how sequelize or mikroorm patch the source object :(

@LennartH
Copy link
Contributor Author

LennartH commented May 6, 2022

Let's just remove the freeze completely. I haven't tested it but I think it will affect how sequelize or mikroorm patch the source object :(

Yes freezing the source object as a side effect, causes issues if you try to store an entity with an ORM library after it was mapped (e.g. to a DTO object). At least when using typeorm, since it adds auto-generated id properties to the object.

I also found that the way the source object was frozen previously (and still in this PR) is shallow, meaning that side-effects in custom mapping configurations and before/after map hooks are still possible, since child objects aren't frozen. To prevent structural side-effects completely we'd have to deep freeze the source object (e.g. by utilising something like lodashs cloneDeepWith).

For now I will adjust my PR so that the source won't be frozen. I don't think that this cause many issues, since side-effects will only happen in user code. If you point me to a good place in the docs I could add a disclaimer that the changes to the source object in user code will "persist".

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@nartc
Copy link
Owner

nartc commented May 6, 2022

@LennartH that would work. In the future we can utilize https://developer.mozilla.org/en-US/docs/Web/API/structuredClone. It landed in Node v17 but hasn't been back ported. As soon as we're on a NodeJS version that supports it, we can start using it.

I also plan on implementing "Type as Contracts" aka Validations against the metadata for incoming Source and outgoing Destination objects so we should be safe that way instead of freezing the objects.

Running CI now. Will merge as long as it's green. Thank you for your contributions

@nartc nartc merged commit a95563d into nartc:main May 6, 2022
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.

Source object is frozen when mapped
2 participants