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

feat(core): add configuration validation using typia #730

Closed
wants to merge 1 commit into from

Conversation

colinaaa
Copy link

@colinaaa colinaaa commented Feb 4, 2025

Important

Note that this is currently just a POC implementation.

Summary

Add a validation to loadConfig to make sure that the configuration is correct when loaded.

I'm using typia and typia-rspack-plugin to make validation.

  1. It use pure TypeScript definition as the source of truth
  2. It is fast

Related Links

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link

netlify bot commented Feb 4, 2025

Deploy Preview for rslib ready!

Name Link
🔨 Latest commit 66e1404
🔍 Latest deploy log https://app.netlify.com/sites/rslib/deploys/67a21d9cf7164b0008abfa07
😎 Deploy Preview https://deploy-preview-730--rslib.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -56,7 +56,9 @@
"rslib": "npm:@rslib/core@0.4.0",
"rslog": "^1.2.3",
"tsconfck": "3.1.4",
"typescript": "^5.7.3"
"typescript": "^5.7.3",
"typia": "^7.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

typia has so many dependencies, this is a bit hard to accept...
image

https://pkg-graph.info/typia

Copy link
Author

Choose a reason for hiding this comment

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

It indeed has too many dependencies, but it is tree-shakable and we are using it as devDependencies.

The bundle size of Rslib is increased from 106.8 kB to 2484.4 kB. But the size change mainly produced by the validation functions generated by typia:

image

As you can see, the src/validate.ts has 2.33 MB in stats. And we can make validation opt-in and lazy loaded.

For end users, no additional dependencies need to be installed, and the bundle size of @rslib/core increases by only approximately 2.5 MB.

Copy link
Member

Choose a reason for hiding this comment

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

If we make the validation opt-in, how can users enable the validation?

Copy link
Author

Choose a reason for hiding this comment

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

E.g.: if the TypiaRspackPlugin(which transforms src/validation.ts) is commented out, the bundle size of @rslib/core in this PR is only 108.4 kB.

image

Copy link
Author

Choose a reason for hiding this comment

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

If we make the validation opt-in, how can users enable the validation?

Maybe through a environment variable?

// rslib.config.js
process.env['RSLIB_CONFIG_VALIDATION'] = true

export default {}

Or maybe a strictDefineConfig API?

import {strictDefineConfig} from '@rslib/core'

export default strictDefineConfig({})

But I would prefer enabling by default. And opt-out using environment variables.

Copy link
Member

Choose a reason for hiding this comment

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

If it's opt-in, I don't think most users will actively enable it...

If it's opt-out, the performance overhead doesn't seem worth it, we can write code to do some validation manually.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'm closing this PR. Thank you for your review

@colinaaa colinaaa closed this Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants