feat: Support excluding fields in ProjectionType #360

CarlosLozanoHealthCaters · 2022-11-15T08:15:54Z

Closes #356

Please take a look! I will include tests and modify the documentation when I have 5 minutes.

avaly · 2022-12-14T15:51:08Z

We appreciate your time on opening this PR and writing all the tests.

We do have some hesitation on introduction the requirement that all projections should be defined as const. We don't see the problem this is solving (being able to send _id: 0) as such a common use case in real world applications (of course we're biased :)) to warrant this breaking change.

If you could refactor the code to work without this requirement, it'd make accepting this PR much easier.

CarlosLozanoHealthCaters · 2022-12-14T23:22:08Z

Hi @avaly

Thanks for your comment.

Although the original problem was suppressing _id, the current change goes further and provides a better alignment with MongoDB projections. In MongoDB, if you want to get all the fields except one, you can put that field with 0 and then the operation will return all fields except the one you don't want. In this way, you don't need to make complex projections or worry about changes in the schema in the future. For me, having this alignment with MongoDB definition is quite interesting and something we should aim for.

About the breaking change, I was playing a bit more with the types and I could find a good solution. If developers doesn't put as const, the projection type consider that all fields defined will be returned. If they put as const, then the projection will work according to the MongoDB definition.

I think this solution brings the best of both worlds. I would like to include some doc about the new projections way if I have your approval.

…ojectionType

avaly

This looks much better! Thanks for the changes and keeping backwards compatibility with the old non-const projections.

We'll definitely need a detailed section about this in our docs. Maybe it fits in the ProjectionType section, since we link to that from all the find* methods.

avaly · 2022-12-19T11:09:46Z

src/utils.ts

+type FilterKeys<T, U> = { [P in keyof T]: T[P] extends U ? P : never }[keyof T];
+type FilterProperties<T, U> = { [K in FilterKeys<T, U>]: T[K] };


Please use longer names for the generic types here. And add some newlines between each type.

CarlosLozanoHealthCaters · 2022-12-20T23:21:56Z

@avaly Please check my latest changes. I added also the documentation. If everything is ok, please go ahead. :)

Have you had time to test it and see it in live?

avaly

This looks great! Thanks for all the work and taking our feedback!

I'll test this internally and merge this after the holidays period.

avaly · 2023-01-10T11:53:26Z

@CarlosLozanoHealthCaters I just got around to trying this on our private repository and I ran into a blocker:

NODE_OPTIONS=--max-old-space-size=6144 yarn tsc
/home/avaly/dev/node_modules/typescript/lib/tsc.js:99699
                throw e;
                ^

RangeError: Maximum call stack size exceeded
    at resolveNameHelper (/home/avaly/dev/node_modules/typescript/lib/tsc.js:41771:35)
    at resolveName (/home/avaly/dev/node_modules/typescript/lib/tsc.js:41769:20)
    at resolveEntityName (/home/avaly/dev/node_modules/typescript/lib/tsc.js:42991:42)
    at getSymbolOfNameOrPropertyAccessExpression (/home/avaly/dev/node_modules/typescript/lib/tsc.js:74339:30)
    at getSymbolAtLocation (/home/avaly/dev/node_modules/typescript/lib/tsc.js:74413:32)
    at getTypeFromTypeNodeWorker (/home/avaly/dev/node_modules/typescript/lib/tsc.js:53916:34)
    at containsReference (/home/avaly/dev/node_modules/typescript/lib/tsc.js:54131:29)
    at visitNode (/home/avaly/dev/node_modules/typescript/lib/tsc.js:25136:24)
    at forEachChildInTypeReference (/home/avaly/dev/node_modules/typescript/lib/tsc.js:25349:20)
    at Object.forEachChild (/home/avaly/dev/node_modules/typescript/lib/tsc.js:25900:47)

Node.js v18.13.0

Any ideas on workarounds?

We're using [email protected]

CarlosLozanoHealthCaters · 2023-01-11T13:42:16Z

@avaly Don't know why that error is being thrown in your code.

I've just run the script 'test:types' from the package.json with yarn test:types and everything is ok.

I have also tested the new papr version in our product and it's also compiling and checking all the types correctly.

Could it be caused by other piece of code?

avaly · 2023-01-12T10:00:25Z

Could it be caused by other piece of code?

No. tsc runs fine on our main branch. As soon as I bring in your changes from Papr, I get the error above. It's definitely a combination of those changes in Papr and some complexity of our internal usage of Papr models.

Unfortunately, this is blocker for us and won't be able to merge this as is. I'll try to debug this in our repo when I get a chance, but I can't give you any timeline.

CarlosLozanoHealthCaters · 2023-01-23T17:42:32Z

@avaly can you check now? I've simplified one of the types and most probably won't throw Maximum call stack size exceeded

avaly · 2023-01-23T17:51:39Z

@CarlosLozanoHealthCaters I checked this branch now with the new commit, but I'm still getting the maximum call stack error.

CarlosLozanoHealthCaters · 2023-01-23T18:03:06Z

Can you provide the tsconfig.json and package.json to check which lib are you using?

avaly · 2023-01-23T18:15:02Z

We're using "typescript": "4.9.3" with this config:

{
  "compilerOptions": {
    "allowJs": true,
    "baseUrl": "./",
    "checkJs": true,
    "esModuleInterop": true,
    "lib": ["ES2020"],
    "module": "CommonJS",
    "moduleResolution": "Node",
    "noEmit": true,
    "noErrorTruncation": false,
    "noImplicitAny": false,
    "noImplicitReturns": false,
    "noImplicitThis": true,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "preserveWatchOutput": true,
    "resolveJsonModule": true,
    "skipLibCheck": true,
    "strictBindCallApply": true,
    "strictNullChecks": true,
    "strictPropertyInitialization": false,
    "target": "ESNext",
    "typeRoots": ["./@types", "./node_modules/@types"],
    "types": ["node", "jest"]
  },
  "exclude": [
    "**/*.cjs",
    "node_modules",
    "**/__tests__/*.js",
    "**/__tests__/**/*.js"
  ],
  "include": [
    "./"
  ]
}

CarlosLozanoHealthCaters · 2023-02-02T16:44:42Z

Could you please check the compilation also crash if you set allowJs to false in your tsconfig?

Probably tons of errors could appear in console, but we can guess if the compilation also crash or not.

CarlosLozanoHealthCaters · 2023-02-03T17:47:07Z

@avaly I created a new type StrictProjectionType to separate the old type and new type. I think it's a good approach and solution.

src/utils.ts

avaly · 2023-02-06T14:51:38Z

@CarlosLozanoHealthCaters I tried this again on our repo, and initially it failed again with the same error - which puzzled me very much. I then tried to comment out your types changes and ran it again, and I got the same error.

At this point, I realized something must be off when trying to use this library from your branch. My method of testing this was that I ran yarn build in the papr git folder, and ran yarn link /path/to/papr in our private repo. This is how I test all incoming PRs to Papr (even our own contributions). For some (unknown) reason, your branch triggers this max call stack error.

So I tried a different approach: yarn pack in the papr git folder, copy the .tgz file to our private repo and use it from there.

Once I did this, your changes no longer trigger that weird error. I even went back to your
4034125 commit, and even that worked (with a few errors popping up on our end due to some generic projection types).

So we could move ahead with either approach, but if you're open to it, I would prefer going with the latest version of the separate StrictProjectionType.

avaly · 2023-02-06T18:33:40Z

I think I changed my mind, so we can try to merge this without the separate strict type.

@CarlosLozanoHealthCaters if you want to revert your commits until this commit 4034125, I can run through it again and finally land this (but probably after v10 goes out #422).

…nges" This reverts commit 7c4a2f0.

CarlosLozanoHealthCaters · 2023-02-07T09:54:52Z

@avaly done!

avaly · 2023-02-14T09:19:57Z

@CarlosLozanoHealthCaters when you have some time, can you please rebase this on the latest main?

CarlosLozanoHealthCaters · 2023-02-15T15:40:41Z

@avaly is it now correct?

CarlosLozanoHealthCaters · 2023-02-15T15:47:19Z

I have simplified

keyof FilterProperties<Projection, 0> | keyof FilterProperties<Projection, 1> extends never

by

keyof FilterProperties<Projection, 0 | 1> extends never

I think it will help compiler

CarlosLozanoHealthCaters added 5 commits December 19, 2022 11:10

feat: suppress _id field and return all but the excluded fields in Pr…

ad57d7b

…ojectionType

chore: Include tests for new projection system

e23a696

chore: Fix documentation

415c114

fix: Include backward compatibility

4ae9fb5

chore: Delete unused imports

f69d2bd

CarlosLozanoHealthCaters force-pushed the fix/projection-type branch from 504f653 to f69d2bd Compare December 19, 2022 10:10

avaly reviewed Dec 19, 2022

View reviewed changes

CarlosLozanoHealthCaters added 3 commits December 21, 2022 00:06

fix: Longer names for generic types

b452be2

Merge branch 'main' into fix/projection-type

b708947

chore: Include docs

b993355

avaly previously approved these changes Dec 21, 2022

View reviewed changes

fix: simplify type

4034125

CarlosLozanoHealthCaters dismissed avaly’s stale review via 4034125 January 23, 2023 17:41

fix: include a new StrictProjectionType to avoid breaking changes

7c4a2f0

fix: space typo

be5c530

avaly reviewed Feb 6, 2023

View reviewed changes

src/utils.ts Outdated Show resolved Hide resolved

add newlines

a750263

CarlosLozanoHealthCaters added 3 commits February 7, 2023 10:49

Revert "fix: include a new StrictProjectionType to avoid breaking cha…

cb57a65

…nges" This reverts commit 7c4a2f0.

fix: Space typo

8a5f3ae

fix: Docs

13d5992

avaly previously approved these changes Feb 7, 2023

View reviewed changes

Merge branch 'plexinc:main' into fix/projection-type

b5d059a

fix: Simplify type

610fa97

CarlosLozanoHealthCaters dismissed avaly’s stale review via 610fa97 February 15, 2023 15:48

avaly approved these changes Feb 15, 2023

View reviewed changes

avaly changed the title ~~feat: suppress _id field and return all but the excluded fields in ProjectionType~~ feat: Support excluding fields in ProjectionType Feb 15, 2023

avaly merged commit 086d8f1 into plexinc:main Feb 15, 2023

avaly added a commit that referenced this pull request Feb 16, 2023

docs: Add docs for #360 in JSDoc

0de60fc

CarlosLozanoHealthCaters deleted the fix/projection-type branch May 7, 2023 07:06

github-actions bot mentioned this pull request Jan 23, 2024

chore(main): Release papr 16.0.0 #720

Closed

		type FilterKeys<T, U> = { [P in keyof T]: T[P] extends U ? P : never }[keyof T];
		type FilterProperties<T, U> = { [K in FilterKeys<T, U>]: T[K] };

feat: Support excluding fields in ProjectionType #360

feat: Support excluding fields in ProjectionType #360

Uh oh!

Conversation

CarlosLozanoHealthCaters commented Nov 15, 2022

Uh oh!

avaly commented Dec 14, 2022

Uh oh!

CarlosLozanoHealthCaters commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avaly left a comment

Choose a reason for hiding this comment

Uh oh!

avaly Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

CarlosLozanoHealthCaters Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

CarlosLozanoHealthCaters commented Dec 20, 2022

Uh oh!

avaly left a comment

Choose a reason for hiding this comment

Uh oh!

avaly commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarlosLozanoHealthCaters commented Jan 11, 2023

Uh oh!

avaly commented Jan 12, 2023

Uh oh!

CarlosLozanoHealthCaters commented Jan 23, 2023

Uh oh!

avaly commented Jan 23, 2023

Uh oh!

CarlosLozanoHealthCaters commented Jan 23, 2023

Uh oh!

avaly commented Jan 23, 2023

Uh oh!

CarlosLozanoHealthCaters commented Feb 2, 2023

Uh oh!

CarlosLozanoHealthCaters commented Feb 3, 2023

Uh oh!

Uh oh!

avaly commented Feb 6, 2023

Uh oh!

avaly commented Feb 6, 2023

Uh oh!

CarlosLozanoHealthCaters commented Feb 7, 2023

Uh oh!

avaly commented Feb 14, 2023

Uh oh!

CarlosLozanoHealthCaters commented Feb 15, 2023

Uh oh!

CarlosLozanoHealthCaters commented Feb 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CarlosLozanoHealthCaters commented Dec 14, 2022 •

edited

Loading

avaly commented Jan 10, 2023 •

edited

Loading