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

Smaller bundle size #258

Closed
wants to merge 12 commits into from

Conversation

HonoluluHenk
Copy link
Contributor

I started on better modularization/build size reduction, references:

This means moving most decorators and their respective validation implementation to their own file.

Since this is very tedious work: please review and tell me if this is the way to go.

Nice side-effect: also adds an easy way to validate with just a function (see ValidateBy.ts).

@@ -2864,7 +2863,7 @@ describe("isPhoneNumber", function() {
someProperty: string;
}

it("should not fail if validator.validate said that its valid", function(done) {
it.only("should not fail if validator.validate said that its valid", function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you remove only?

@henrikra
Copy link
Contributor

@HonoluluHenk Are you still alive? :D

@HonoluluHenk
Copy link
Contributor Author

Sorry, currently on holidays ☺️

@HonoluluHenk
Copy link
Contributor Author

Since this introduces an API change (changed import) and requires lots of tedious work for me to implement this for all validator functions/annotations, I'd like to have the opinion of the package maintainer...

Care to comment, @NoNameProvided?

@NoNameProvided
Copy link
Member

Hi @HonoluluHenk!

Yes, this is great work! I will help with the changes as my time allows. I have been inactive for a while now, but now I managed to spend time on OSS again (last week I have caught up with the changes in class-transformer, this week I will catch up with this lib. I will prioritize answers on issues first, but expect me to look/help with this on the weekend.)

@henrikra
Copy link
Contributor

henrikra commented Nov 4, 2018

@NoNameProvided My pr would be easy to check out too :)

@henrikra
Copy link
Contributor

henrikra commented Nov 5, 2018

@HonoluluHenk Can you fix conflicts?

@HonoluluHenk
Copy link
Contributor Author

@NoNameProvided did some more migrations, but found a bug:
"context" is (imho) not implemented for custom validators.
I fixed this in 86079d8 but this commit needs proper review!

@HonoluluHenk
Copy link
Contributor Author

@NoNameProvided: migration done.
Most of the changed files are just the decorators and validation functions moved to their own file.
Files that require your special attention:
validation/ValidationExecutor.ts (I left one FIXME in validateOrReject)
validation/ValidationTypes.ts
validation/Validator.ts (validateValueByMetadata is gone since all metadata-validations are migrated to ValidateBy())
validation-schema/ValidationSchemaToMetadataTransformer.ts

@HonoluluHenk
Copy link
Contributor Author

Measures with a minimal angular application:
Empty angular project without class-validator: 312k

Using class-validator 0.9.1: bundle total 864k

Compare to this refactoring:
Bundle size when adding class-validator: 328k (just using validate() and some simple type checkers)
Including @IsPhoneNumber: 772k (this is still less than v 0.9.1!)

@henrikra
Copy link
Contributor

Wow I really like this. How do you measure bundle size btw?

@Djaler
Copy link

Djaler commented Sep 25, 2019

@maxime1992 in angular cli one can not change webpack config? In vue cli we can do it easily

@maxime1992
Copy link

@maxime1992 in angular cli one can not change webpack config? In vue cli we can do it easily

You could eject or use ngx-build-plus for ex but it's definitely not worth it in this case as I do not use the library at all and a fix is coming. So I decided to go for the ugly quick fix there :)

@tmtron
Copy link
Contributor

tmtron commented Jan 2, 2020

To get rid of libphonenumber for the angular-cli I came up with a hack. The idea is a combination of the comments from maxime1992 and Djaler

In your workspace save this libphonenumber-stub.js file:

module.exports = {
    PhoneNumberUtil: {
        getInstance() {
            return null;
        },
    },
};

in package.json use the postinstall hook to replace the original lib:

{
  "scripts": {
    "postinstall": "npx nodecat libphonenumber-stub.js > ./node_modules/google-libphonenumber/dist/libphonenumber.js || true"
   }
}

hint: we use npx nodecat (instead of plain cat) so that this also works on Windows

@gempain
Copy link

gempain commented Feb 25, 2020

@NoNameProvided any chance this PR would be merged in ?

@patrykkrawczyk
Copy link

please merge this @vlapo @NoNameProvided @quentinus95

@gempain
Copy link

gempain commented Mar 29, 2020

@HonoluluHenk would you be able to rebase your changes on the latest target branch so the conflicts are resolved ?

@vlapo
Copy link
Contributor

vlapo commented Mar 30, 2020

🚀 I would like to ask you all for help in testing and reviewing PR #568 🚀

@vlapo
Copy link
Contributor

vlapo commented Apr 3, 2020

@henrikra @gempain @patrykkrawczyk @maxime1992 @Djaler @ramyothman Could you please test preview release (#568) and provide some feedback?

@gempain
Copy link

gempain commented Apr 3, 2020

@vlapo yes, I'll give it a shot. Actually, had a chance yesterday to try 12.0.0.rc-3 with jest and I had a few issues, but it was not a clean install so I need to see if I can reproduce on a clean install because I am wondering whether the problem was jest. Give me a day or two so I can do this. Thanks for the time you're putting into this, very appreciated.

@gempain
Copy link

gempain commented Apr 4, 2020

@vlapo I've put together a demo repo, and so far I can't get tree shaking to work with 0.12.0.refactor-4. I followed webpack's official tree shaking guide which tells you to set "sideEffects": false" in package.json and optimization.usedExports to true in webpack.config.js, but I'd be greatful if someone can take a look at the repo and see if everything seems configured properly.

@vlapo feel free to use my repo to test out your changes or fork it. Do you want me to add you as a contributor so you can commit there ?

@patrykkrawczyk
Copy link

it's also not tree-shakebale in my project
without class-validator: 4 kb
with class-validator: 903 kb
this is using 0.12.0.refactor-4 and only doing import of IsString decorator

@vlapo
Copy link
Contributor

vlapo commented Apr 4, 2020

Thank you for testing! 0.12.0-refactor.5 is published.
@gempain I test it on your demo app. But I think you have to update tsconfig and add:

"module": "es2015",
"moduleResolution": "node",

@patrykkrawczyk could you test 0.12.0-refactor.5? What is your environment? Do you use webpack in your project?

@gempain
Copy link

gempain commented Apr 5, 2020

@vlapo after updating my tsconfig and upgrading to reafctor 5, it is now tree shakable ! Great job !

@patrykkrawczyk
Copy link

0.12.0-refactor.5 is tree shakeable in my project.
I'm writing AWS Lambda function using Serverless Framework, Node 12 and TS.
Without class-validator: 4.88KiB
With class-validator isString: 20.8 KiB
With class-validator @IsString: 21.6 KiB
With class-validator @IsPhoneNumber('DE'): 661 KB

@gempain
Copy link

gempain commented Apr 5, 2020

@vlapo I think after this, we'll have to refactor the @IsPhoneNumber validator. I can't believe we're embedding 400kb just for validating a phone number. We'd need to drop libphonenumber.js.

@vlapo
Copy link
Contributor

vlapo commented Apr 7, 2020

@gempain this PR is about keeping libphonenumber.js. After #568 PR merged we can use any lib (even a large one) for validation as tree shaking will remove unused code.

@gempain
Copy link

gempain commented Apr 7, 2020

@vlapo sure, now people that don't use the phone validator won't get those 400kb, but people that do use it will get those kb, and I find it ackward that the validation logic of the phone validator weighs as much as the entire library. Anyway, that's another matter.

@vlapo
Copy link
Contributor

vlapo commented Apr 7, 2020

Of course. We have to inform users about this disadvantage (as a note in docs). Developers can use validatorjs IsMobilePhone validator as alternative. IsPhoneNumber is some kind of heavy gun 🔫

But if someone know replacement for libphonenumber.js I am open for suggestions 🚀

@vlapo vlapo closed this in #568 Apr 17, 2020
vlapo added a commit that referenced this pull request Apr 17, 2020
BREAKING CHANGE: 

Validation functions was removed from `Validator` class to enable tree shaking.

BEFORE:

    import {Validator} from "class-validator";

    const validator = new Validator();
    validator.isNotIn(value, possibleValues);
    validator.isBoolean(value);

AFTER:

    import {isNotIn, isBoolean} from "class-validator";

    isNotIn(value, possibleValues);
    isBoolean(value);

Closes #258, #248, #247, #212
@desmap
Copy link

desmap commented Jul 8, 2020

Pls check a part of my bundle graph:

image

Is this normal? I've import { IsMongoId, MaxLength, ArrayMaxSize } from 'class-validator and import { validate } from 'class-validator in my codebase.

My tsconfig matches the one from @gempain and my webpack conf is the one from Next + usedExports: true and I added "sideEffects": false" in package.json.

FWIW, this is a Next production build of the client side. I have v0.12.2 of class-validator.

@ramyothman
Copy link

ramyothman commented Jul 10, 2020 via email

@jbjhjm
Copy link

jbjhjm commented Jul 13, 2020

@ramyothman should not be the case in most recent versions anymore (treeshakable validators). As you can see in @desmap 's chart, libphonenumber is not in there. I guess he/she is wondering why so many validators are bundled as just 3 were imported.

@NoNameProvided
Copy link
Member

Just FYI, the package used for validating phone numbers has been replaced in the current develop branch with a package using the same meta information to validate the phone number but much smaller.

@github-actions
Copy link

github-actions bot commented Sep 3, 2020

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet