feat: exclude with groups and versions (closes typestack/class-transformer#202)

[lucas-labs]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Closes typestack#202
Right now using groups and version is only posible with @Expose decorator.

Issue Number: typestack#202

What is the new behavior?

It allows to use groups and version with the @Exclude decorator aswell.
Now @Exclude and @Expose share a more similar API.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[Aareksio]

Using the opportunity, what is the plan with nestjs/class-transformer fork? Is it planned to be actively maintained within NestJS ecosystem (at the very least accepting PRs and making new released), or forked just for convenience? I've been struggling with the issue this PR solves as well as a few others for years, but with typestack ecosystem being unmaintained for years now, there was little chance of having PRs merged.

[Aareksio]

If I read this correctly, it is going to NOT exclude the property if group check passes, but version check fails.

class Item {
  @Exclude({ group: ['group'], since: 3 })
  id: number;
}

const item = new Item()
item.id = 1

console.log(classToPlain(item, { group: ['group'], version: 2 })) // Item { id: 1 }

[lucas-labs]

Hi! Thanks for the review.
As you said, it will not exclude the property if the version check fails, even if the groups matches.
In your example, it will only exclude the property if version >= 3. This means version takes prevalence over groups.
If I remember correctly, it works the same way for Expose, but I could be wrong about that.

[Aareksio]

I understand this is a design choice.

This is still much better than not having the ability to exclude properties by group. I am absolutely positive it should be merged even if this behaviour does not satisfy all needs, make class-transformer better one step at the time. That said, let me introduce a real life example of an issue we face in current project:

We want to share a limited representation of our object with a 3rd party integration:

class Ticket {
  @Exclude({ group: ['external'] })
  priority: number;
}

Now in the future version of the API, we may want to deprecate the priority altogether:

class Ticket {
  @Exclude({ group: ['external'], since: 2 })
  priority: number;
}

With current logic in place, due to AND conjunction, this would expose priority for API v1 group external.

The problem can be solved in multitude of different ways, this is likely not the place to discuss them, I just wanted to clarify current implementation is a design choice and highlight it's downfalls.

[lucas-labs]

Actually, it's not an AND conjuction. It's more that version takes precedence over groups.

class Ticket {
  @Exclude({ groups: ['external'], since: 2 })
  priority: number;
}

In v1, it would be exposed for everyone

classToPlain(ticker, { groups: ['external'], version: 1 });
// {priority: 1}
classToPlain(ticker, { version: 1 });
// {priority: 1}

Since v2 it will be excluded for all groups, because version takes precedence over groups:

classToPlain(ticker, { groups: ['external'], version: 2 });
// {}
classToPlain(ticker, { version: 2 });
// {}

Maybe it should be at least an AND conjuction. I'm not sure how Expose works in this situation, I haven't used groups and version together. I will take a look at Expose and do some testing tomorrow (it's almost 12pm here 😂)

[kamilmysliwiec]

Thanks for your contribution.

We decided to fork the original package to keep pushing security improvements, upgrading dependencies, and addressing potential vulnerabilities. We will also consider merging performance improvements and new features in case they could significantly improve the DX & make things way simpler. However, we won't be merging minor enhancements and improvements, nor extra decorators/things that are already doable with this package, especially if they introduce breaking changes (as we plan to introduce as few API breaking changes as we can).

This one doesn't introduce a breaking change + it looks like a useful improvement so we can consider merging it in.

[oktapodia]

@kamilmysliwiec Do you think this can be merged soon?

[stevenolay]

Watching this with care! Would be very useful!

[rgoupil]

A great feature, surprisingly missing from the original package, thanks!

@lucas-labs and @Aareksio what's the reasoning behind pointing this PR to the https://github.com/@nestjs/class-transformer fork instead of the main package https://github.com/typestack/class-transformer?
It seem like this fork in only kept for the nest team convenience and not intended to be used outside (starting with the readme still instructing users to download the original package).

I just happen to fall on a use case for this PR and would love to be able to remove the workaround.
Any chance to retarget the main repo instead of this fork?

[lucas-labs]

Hi @rgoupil, I created this PR on this fork because the original project was no longer being actively maintained. When I created this PR the project seemed completly abandonded, last release had been like a year before that. Now they are releasing some updates, but from what I see, they are only merging security fixes, not new features or bugfixes.

[rgoupil]

Oh, indeed, I hadn't noticed 😞
@kamilmysliwiec, I noticed you updated the README 3h hour ago, following the mention of the outdated installation instructions, maybe the NestJS team could be interested in this PR 😃 ?

[dawid-jazdzewski-BO]

Are there any plans according this PR? I would love to see groups property in @exclude decorator