-
Notifications
You must be signed in to change notification settings - Fork 30
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
[In progress] Modernize Build tooling, optimize code size #65
Conversation
Remove bower Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Hi @rxaviers, I finished the initial implementation of the changes. You can start reviewing if you feel like it. I still need to add a few benchmarks on what you can expect for the final size with all the "validate" calls removed in production and do some tests to make sure that everything works as it did before. I recommend you look at the commits separately, I tried to make them in the right order and well separated, otherwise, looking at all the changes at once might give you a headache :) I also recommend checking the box "Hide whitespace changes" to have less false positives of line changes. |
Awesome! Thank you for your contribution. One highlevel comment for now: Mocha doesn't need a browser to run either. Can we do the analogous test changes you did but keeping mocha? Thanks |
Oh, I was sure of the contrary. my mistake :) I will revert that part then. I have to admit I'm more familiar with Jest, but I'll figure it out :) |
Also feel free to get rid of grunt and instead use npm scripts directly. |
About mocha, follow an example https://github.com/rxaviers/async-pool/blob/master/package.json#L8 (also check https://github.com/rxaviers/async-pool/blob/master/test/mocha.opts). |
Good to know for Grunt, but there is no cli for grunt-dco / dco . Do you want to keep that plugin ? Thanks for the links for Mocha |
Good point! We can leave only that in grunt for now until a CLI is made available for it, which shouldn't be hard to create (that hypothetical module could use https://github.com/scottgonzalez/dco) |
sure, I already checked how hard it would be to create that, but I wanted to make sure that you still want it first :) |
Thanks for checking! |
Oh and I wanted to ask, do you wish to keep jshint or to use eslint ? |
Let's go with eslint + prettier (we could use https://github.com/rxaviers/async-pool/blob/master/package.json settings as baseline for that too) |
aaand, mocha and eslint it is :) |
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
…the same Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
…apped in if (process.env.NODE_ENV !== "production") Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
Signed-off-by: Stéphane Goetz <onigoetz@onigoetz.ch>
I also made a small script to replace grunt-dco and removed grunt |
"yarn build" or "npm run build" builds everything now |
I made some tests on the size of the library
It seems the UMD build is bigger, but the global size of the build with webpack is smaller. |
What you have done here is already a big improvement and therefore I am merging it. I still want to do additional things with respect to the build and modules. Will post details in an upcoming PR. |
Follow up #66 |
Hello,
as discussed in the Globalize repository, this pull request will make a version that is optimizable by build tools like webpack.
But there is a lot of work required to get there due to the tools used currently
I open this pull request early so that you can see the progress and give your feedback if you're ok with the direction it's taking
validate*(
with conditions to remove them in production builds