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

Converted moment, moment range, and Vue to peer dependencies #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spgoad
Copy link

@spgoad spgoad commented May 3, 2019

This somewhat relates to issue #116 and package size as this was discussed as a possible approach for solving that problem as well.

We we're working on our first deployment using the vue-ctk-date-time-picker, and I noticed our chunk-vendors.js compiled by Webpack was pretty big. After digging into it, I discovered that this package was importing moment, vue, and moment range as npm dependencies, which means they are included in the final dist js files. This would be ok, except our app also has moment and vue as dependencies - which means we've now included them in our final package twice by including the dist version of vue-ctk-date-picker.

This PR converts VueJS, Moment, & Moment-Range to peer dependencies which allows us to only include each of them one in our application. This would be a somewhat significant change as anyone who upgrades would need to install the 3 dependencies themselves - so I welcome feedback on the approach and how that would need to be communicated in docs.

@jkcs
Copy link
Contributor

jkcs commented May 4, 2019

hey guys!I want to ask you a question that sounds like a novice, but I've never used it.
Change devDependencies to peerDependencies by modifying package. JSON directly or by command.But I couldn't find such an order in the official documents.I don't think it's a direct modification of package. json, I haven't found the command for a long time.I hope you can answer me. Thank you.

@spgoad
Copy link
Author

spgoad commented May 10, 2019

@lw66666 I don't believe there is a way to do it with a command. The way I went about it is to do npm install package-name --save-dev to save it to dev dependencies, and then I manually add it to peer dependencies. This way I have the dependency installed locally for testing purposes, but then when the project is built for distribution, the dependency is not included in the final package.

@jkcs
Copy link
Contributor

jkcs commented May 12, 2019

@spgoad Thank you.

@ghost
Copy link

ghost commented Jul 1, 2019

I think this is a great idea, as keeping the bundle size down would be very welcome. I'm not sure how you tackle existing installations, but think this is definitely worth considering.

@m-mohr
Copy link

m-mohr commented Aug 21, 2019

I'd like this PR to be merged, although it would need to be released as v3.0 as it's a breaking change.

@WilliamDASILVA
Copy link
Contributor

Thank you for taking the time making this PR. Indeed, removing those dependencies would be very nice for the bundle. Yet as said by @m-mohr it may break current versions. I'll do some researching in the following days to tackle this issue.
Before doing so, I need to add a proper test suite to avoid silly regressions.

I'll keep you updated.

@guillaumebriday
Copy link

Hey!

What is the reason to not merge this PR ?

It looks good to me

Thanks

@tylercollier
Copy link

I'd love to see this PR merged. My bundle jumped from 159KiB to 547KiB by including this package (JS only, excludes the CSS). I wish I would have known about this when choosing which date picker to choose.

@m-mohr
Copy link

m-mohr commented Nov 12, 2020

Yeah, I think we are going to switch date pickers, too. The size can't be justified any longer...

@guillaumebriday
Copy link

guillaumebriday commented Nov 12, 2020

I switched to https://element.eleme.io/#/en-US/component/datetime-picker#datetimepicker

The api is better too imo

@Tofandel
Copy link

Tofandel commented Jun 16, 2021

Please have this merged... 800Kb gzipped for this bundle this is crazy.. Vue shouldn't be bundled, and moment as well because this prevents us from removing the locales which are unnecessary with the dedicated webpack plugin...

I have never seen such a big npm package, this should be addressed with top priority
image

Here is the size of another package that does the same thing
image

@m-mohr
Copy link

m-mohr commented Jun 16, 2021

I've migrated to https://github.com/mengxiong10/vue2-datepicker, which gives me a much better experience overall.

@Tofandel
Copy link

I did the same, also very happy now

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.

7 participants