typescript-runtime-type-benchmarks icon indicating copy to clipboard operation
typescript-runtime-type-benchmarks copied to clipboard

feat: adds @deepkit/type

Open BasixKOR opened this issue 3 years ago • 10 comments

Making a draft since I had a hard time running this on my local machine. Aims to resolve #894.

BasixKOR avatar Jun 17 '22 16:06 BasixKOR

Also, deepkit seems to require at least node v14 (tried that locally and it worked):

    /home/runner/work/typescript-runtime-type-benchmarks/typescript-runtime-type-benchmarks/node_modules/@deepkit/type/dist/cjs/src/reflection/reflection.js:923
                type.visibility = prop.visibility ?? type_1.ReflectionVisibility.public;
                                                   ^
    SyntaxError: Unexpected token '?'
      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14)
      at Object.<anonymous> (node_modules/@deepkit/type/dist/cjs/src/decorator.js:4:28)

To resolve this, we could:

  1. add a node version filter to the benchmarks
  2. use some tool (babel?) to transpile node-modules
  3. drop node 12 benchmarks

In absence of other options, I'd vote for dropping node 12 benchmarks :grin:

hoeck avatar Jun 18 '22 10:06 hoeck

@BasixKOR I you like, you can also run the tests locally with npm run test, you'll then see that the parseStrict case test is failing for deepkit:

    deepkit
      ✓ should validate the data (2 ms)
      ✓ should throw on invalid attribute type (1 ms)
      ✕ should throw on extra attributes
      ✕ should throw on extra nested attributes
      ✕ should throw on missing attributes

If there is no strict validation option for deepkit (I did not found any, tried validatedDeserialize() but that did ignore extra attributes), you can omit the case and only include those that deepkit supports.

hoeck avatar Jun 18 '22 10:06 hoeck

use some tool (babel?) to transpile node-modules

@hoeck Yeah, It seems like Jest allows to do this with transformIgnorePatterns so I think I'll try this option.

you can omit the case and only include those that deepkit supports.

~~Thanks, I'll remove loose and safe test cases for now until I figure out how to ignore unknown keys.~~

I understood in a completely opposite way :joy: I'll remove the strict ones, thanks for the suggestion!

BasixKOR avatar Jun 18 '22 11:06 BasixKOR

I understood in a completely opposite way joy I'll remove the strict ones, thanks for the suggestion!

Indeed that was worded quite ambiguously, I meant removing the benchmark case :sweat_smile: not the test case.

Yeah, It seems like Jest allows to do this with transformIgnorePatterns so I think I'll try this option.

That will help with running the Jest tests but you'd still need a separate setup for running the benchmark?

As the oldest LTS release is node 14 now, I am all in favor of dropping node 12 support instead of introducing more build complexity. @moltar any opinions?

hoeck avatar Jun 18 '22 11:06 hoeck

I am in favour of dropping Node v12!

moltar avatar Jun 18 '22 13:06 moltar

I am in favour of dropping Node v12!

:+1: okay I'll create a PR that drops node 12, after merging, this PR can then be rebased and deepkit tests and benchmarks should work :grinning:

hoeck avatar Jun 24 '22 06:06 hoeck

@BasixKOR could you please rebase on the latest master - I have removed node 12, the benchmarks now run on 14, 16 and node 18 only so your PR should pass after the rebase. If you don't have time any more, please let me know and I will take care of this PR. Thanks!

hoeck avatar Jul 14 '22 15:07 hoeck

I rebased the deepkit branch and it should be okay, please lmk if anything is a bit off

BasixKOR avatar Jul 15 '22 13:07 BasixKOR

Fixed some remaining rebase issues and uses guard and deserializeFunction now! Unfortunately 'should throw on missing attributes' test is breaking though.

BasixKOR avatar Jul 16 '22 10:07 BasixKOR

'should throw on missing attributes' test is breaking though

Could you skip that benchmark then? It is okay if you only implement the benchmarks that deepkit supports.

I created separate benchmarks as checking for missing attributes adds some overhead and I wanted to be able to compare libs that perform that extra step.

hoeck avatar Jul 20 '22 12:07 hoeck