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

Use peer dependency for Luxon #355

Closed
Enngage opened this issue Jul 1, 2020 · 10 comments
Closed

Use peer dependency for Luxon #355

Enngage opened this issue Jul 1, 2020 · 10 comments

Comments

@Enngage
Copy link

Enngage commented Jul 1, 2020

Hey there!

Could you please make Luxon a Peer dependency? Since you have it listed as dependency it is shipped with your library and if the project is also using Luxon, it will result in code duplication and +60KB because multiple Luxon libraries can be included.

Using Peer dependencies will mitigate this problem completely.

@Agranom
Copy link
Owner

Agranom commented Jul 1, 2020

Hey

I had issue to make it as dependency. I'm afraid if I change it back there will be more issues )

@Enngage
Copy link
Author

Enngage commented Jul 1, 2020

Thanks for a quick reply :-) I'm pretty sure there wouldn't be any issues (I've been using it for some time for several of my packages). It would be the same thing as Angular itself does with rxjs. What you would do is:

  • Specify Luxon (ideally with minor relaxed versioning) as a dependency (e.g. "luxon": "^1.24.0")
  • Move Luxon to devDependencies and peerDependencies in this repo as it is needed to build & run your code (moving it to devDependencies basically means it will install for you, but not for people using your library which is what you want to prevent your users from having duplicate Luxon libraries). There is pretty much no different for you if you install packages in your dependencies or devDependencies so it wouldn't break anything. It only affects what code is distributed alongside your code.
  • Update your docs to include Luxon installation notes:
npm i --save ngx-material-timepicker
npm i --save luxon

Npm will tell people automatically to install peer dependencies when running npm i, but its good practice to include it in docs.

Btw: thank you for maintaining the library. We've tried probably all time selector for Angular and this one is the best we found.

@Enngage
Copy link
Author

Enngage commented Jul 2, 2020

luxon

Adding a bundle report for reference.

@Agranom
Copy link
Owner

Agranom commented Jul 2, 2020

@Enngage Thanks for explanation. Definitely will do it soon.

@Enngage
Copy link
Author

Enngage commented Nov 15, 2020

Any chance you can look at this? It should be really easy to implement - just move it to peerDependencies and it would save a lot of people at least 60KB from their bundles ;)

@Enngage
Copy link
Author

Enngage commented Feb 2, 2021

Still nothing? This is an easy fix that takes about 1 minute to implement and saves 60KB for all users using your library in combination with luxon which is the best alternative for moment at this time...

@maxstee
Copy link

maxstee commented Feb 12, 2021

I'm also quite bothered by this.
immagine
I'm considering using date-fns to avoid that.

@Enngage
Copy link
Author

Enngage commented Feb 12, 2021

There is probably no point in waiting anymore since fix that takes 1 minute to implement is here from last July. We are moving away from this library and instead created our own time picker. Gl anyways.

@edemircan
Copy link

Would be great. Could shrink my app with a few kb.

@Agranom
Copy link
Owner

Agranom commented Jun 14, 2023

Done. Pls update to the latest version. Sorry for being so long

@Agranom Agranom closed this as completed Jun 14, 2023
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

No branches or pull requests

4 participants