Skip to content

Conversation

@jonluca
Copy link

@jonluca jonluca commented Aug 9, 2022

I removed the dependency on ajv for har validation, which should make this library more usable on the web, as well as improve bundle size.

I also migrated the test suite to vitest.

@@ -1,14 +0,0 @@
import { request } from '../targets/node/request/client';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was unused

// Jest Snapshot v1, https://goo.gl/fbAQLP
// Vitest Snapshot v1

exports[`availableTargets > returns all available targets 1`] = `
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is vitests snapshot

@dimitropoulos
Copy link
Contributor

thanks! this is certainly interesting!

The thing that worries me about removing the ajv validation is that it's extremely critical to all subsequent codepaths that the input is a valid HAR. If you replaced it with hand-rolled code that did the same checks as AJV I'd be totally cool with that, but it looks like it was just outright removed. Did I read that correctly in the diff?

@dimitropoulos
Copy link
Contributor

Also, could you please say more about the advantages you see with switching to vitest? I'm certainly open-minded on the topic, but I don't know what we substantively gain by making the switch. For example, can you please post some before-and-after runs for any performance differences so I can compare to my own runs?

@jonluca
Copy link
Author

jonluca commented Aug 30, 2022

So ajv has a compiled mode - it uses the exact same logic as normal ajv, but creates a single binary with no dependency on ajv. You can see what the compiled code looks like here https://github.com/jonluca/har-validator/blob/main/src/validate.js

This is exactly the same logic that ajv and the har schema would be using, just compiled at build time instead of runtime. We should see bundle size improvements and runtime speed improvements by using the compiled version.

@jonluca
Copy link
Author

jonluca commented Aug 30, 2022

For example, can you please post some before-and-after runs for any performance differences so I can compare to my own runs?

Sure, here's a screenshot

The nice part is that 1) it has an enormous emphasis on speed (as the entire vite toolchain does) and 2) it comes with support for code coverage reporting out of the box, in a way that's faster and nicer (IMO) to jest.

Screen Shot 2022-08-29 at 8 58 57 PM

@jonluca
Copy link
Author

jonluca commented Aug 30, 2022

And vitest is a much smaller and more lightweight library to jest, it'll improve npm install times as well.

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2024

CLA assistant check
All committers have signed the CLA.

@filfreire
Copy link
Contributor

Closing as stale

@filfreire filfreire closed this Jul 12, 2024
@jonluca
Copy link
Author

jonluca commented Jul 12, 2024

Are you not interested in using the compiled one? It would improve the install size significantly, and speed it up as well

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.

4 participants