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

fix(build): import Flatpickr Locale on demand via regular imports #227

Merged
merged 3 commits into from
Jan 5, 2021

Conversation

ghiscoding
Copy link
Owner

  • we shouldn't use require(locale) because they all end up being part of the final bundle, it's better to let the user import whichever Flatpickr Locale he wants to use and that will end up being a much smaller bundle with only the locale we really need

- we shouldn't use require(locale) because they all end up being part of the final bundle, it's better to let the user import whichever Flatpickr Locale he wants to use and that will end up being a much smaller bundle with only the locale we really need
@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #227 (193e1e4) into master (e35f116) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #227   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          207       207           
  Lines        11847     11829   -18     
  Branches      4064      4058    -6     
=========================================
- Hits         11847     11829   -18     
Impacted Files Coverage Δ
packages/common/src/editors/dateEditor.ts 100.00% <100.00%> (ø)
packages/common/src/filters/compoundDateFilter.ts 100.00% <100.00%> (ø)
packages/common/src/filters/dateRangeFilter.ts 100.00% <100.00%> (ø)
...bundle/src/components/slick-vanilla-grid-bundle.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e35f116...193e1e4. Read the comment docs.

@zewa666
Copy link
Contributor

zewa666 commented Jan 5, 2021

This is definitely a better approach since you aint really gonna need all locales bundled all the time.

Tbh this actually is a breaking change, because users depending on a different language would now have to touch code as their CI/Tests might break. It feels relatively small but essentially it still is one ;) dunno If you just want to document this which might be enough given the en fallback

@ghiscoding
Copy link
Owner Author

hmm yeah it might be a small breaking change but I certainly won't do a version 4.x lol
I will document it in Wikis and the Release, to be fair there's a warning message so they will eventually see what to do and there's probably a low number of developers using multiple locales.

@zewa666
Copy link
Contributor

zewa666 commented Jan 5, 2021

Well you asked me and you got an answer ;)
I'm by god no overzealos semver fanatic so I'd tend to go with what you said that the benefit and clear/simple fix should be ok

@ghiscoding
Copy link
Owner Author

haha I do appreciate your feedback since you're a big user of the lib 😉

@ghiscoding ghiscoding merged commit 6644822 into master Jan 5, 2021
@ghiscoding ghiscoding deleted the bugfix/flatpickr-locale-bundling branch January 5, 2021 16:34
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.

2 participants