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(client-core): dayjs locale conflict #3606

Merged
merged 2 commits into from Nov 2, 2021
Merged

Conversation

LvtLvt
Copy link
Contributor

@LvtLvt LvtLvt commented Oct 30, 2021

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Hii^^ I'm using cubeJS for analytics API^^
First of all, thank you for creating a really great platform. thanks to you, I'm so into cubeJs these days..

Description of Changes Made (if issue reference is not provided)

if I change the locale value of dayjs on the front-end, no value is displayed when granularity is week.

Currently, if you look at the ResultSet code, the weekStart must be set to 1 to work properly.

However, the value can be modified by the client as much as possible (https://day.js.org/docs/en/i18n/changing-locale)

so it seems necessary to guide or cope with it.

To reproduces the problem, you can overwrite the locale of dayjs with ko or another value and test it.
please refer to the test code..

thank you^^

@LvtLvt LvtLvt requested a review from a team as a code owner October 30, 2021 12:50
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Oct 30, 2021
@LvtLvt LvtLvt changed the title Add internal dayjs proxy Fix dayjs locale conflict Oct 30, 2021
@LvtLvt LvtLvt changed the title Fix dayjs locale conflict fix: dayjs locale conflict Oct 30, 2021
@codecov
Copy link

codecov bot commented Oct 31, 2021

Codecov Report

Merging #3606 (89bf4f0) into master (cc907d3) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3606      +/-   ##
==========================================
- Coverage   59.70%   59.67%   -0.04%     
==========================================
  Files         125      125              
  Lines       10007    10014       +7     
  Branches     2263     2266       +3     
==========================================
+ Hits         5975     5976       +1     
- Misses       3750     3756       +6     
  Partials      282      282              
Impacted Files Coverage Δ
packages/cubejs-backend-shared/src/time.ts 28.57% <0.00%> (-1.28%) ⬇️
...es/cubejs-server-core/src/core/RefreshScheduler.ts 70.31% <0.00%> (-0.75%) ⬇️
...y-orchestrator/src/orchestrator/PreAggregations.ts 83.88% <0.00%> (-0.11%) ⬇️

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 cc907d3...89bf4f0. Read the comment docs.

@paveltiunov
Copy link
Member

@LvtLvt Hey David! Looks great! The only minor thing it seems Proxy can be replaced by a function as we don't use much of API from dayjs currently.

@LvtLvt
Copy link
Contributor Author

LvtLvt commented Nov 2, 2021

@paveltiunov Hii^^ thank you for review. I think you are right. proxy code seems overkill.. your comment helped me a lot..

@vasilev-alex vasilev-alex changed the title fix: dayjs locale conflict fix(client-core): dayjs locale conflict Nov 2, 2021
@paveltiunov
Copy link
Member

@LvtLvt Looks great! Thanks for contributing it!

@paveltiunov paveltiunov merged commit de7471d into cube-js:master Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants