fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(isDate): Timezone Offset Fix - #2257

Merged
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master
Aug 18, 2023
Merged

fix(isDate): Timezone Offset Fix#2257
profnandaa merged 3 commits into
validatorjs:masterfrom
tomaspanek:master

Conversation

@tomaspanek

Copy link
Copy Markdown
Contributor

This pull request resolves issues with false negatives when using the isDate validator (Issue #2256). When parsing dates, the YYYY-MM-DD date format gets interpreted to be in the UTC time zone (MDN Docs), while the getDate() method outputs the day of the month in the current time zone (MDN Docs). This apples-to-oranges comparison can result in occasional discrepancies, depending on the user's local time and timezone.

This PR appends T00:00:00 time information into the parsed string (as suggested by @brunoabude). That will cause the date string to be interpreted in the local time zone. Additionally, I added leading zeros for months and days (where applicable) to ensure that date strings always follow the ISO 8601 format (as intended in #2231).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@codecov

codecovBot commented Aug 5, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f074abd) 99.95% compared to head (873ed37) 99.95%.

Additional details and impacted files
@@ Coverage Diff @@## master #2257 +/- ##
=======================================
Coverage 99.95% 99.95% =======================================
Files 107 107 Lines 2436 2442 +6 Branches 615 617 +2 =======================================
+ Hits 2435 2441 +6 
Partials 1 1 
Files ChangedCoverage Δ
src/lib/isDate.js100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests to ensure that we don't break this validator again

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

I added in a test that mocks a different time zone. Passes when the isDate validator is implemented correctly.

Additionally, I found out that timestamps ending T00:00:00 are parsed differently before ES6/Node 8. Details here. To ensure maximum compatibility, dates are now getting parsed as full UTC strings (the getUTCDate() method then ensures the day of the month output is the same is input).

@brunoabude

brunoabude commented Aug 5, 2023

Copy link
Copy Markdown

Nice catch on this ES2015 thing! It actually makes way more sense to use the full format with UTC+0 offset. I've also cherry-picked just the test with timezone-mock to the current master branch and confirmed that is failing with the current implementation.

Btw, there is a note on the timezone-mock package page that says:

Note: Future timezone transitions are likely to change due to laws, etc. Make sure to always test using specific dates in the past. The timezone data used by timezone-mock 1.0.4+ should be up accurate for all times through the end of 2018.

Would be nice to include dates up to 2018 to make the tests more resillient!

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

Excellent point! I adjusted the unit test accordingly.

@tomaspanek
tomaspanek requested a review from WikiRikAugust 5, 2023 18:00

@WikiRikWikiRik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the leading zeroes are actually needed, but using getUTCDate() instead of getDate() is fine with me.

The Node 6 test is failing, but it's not due to this PR

@WikiRik

Copy link
Copy Markdown
Member

@profnandaa could you take a look at this and maybe get a patch release out since this bug got introduced by the latest version?

@profnandaa

Copy link
Copy Markdown
Member

Anyone knows why this is failing on Node.js v6? We still need to support this, was helping us as proxy for some backward compatibility on browsers, thought I know in prod, there could be no one still using Node v6.

@profnandaa

Copy link
Copy Markdown
Member

Would like to get this in once that is sorted. Will appreciate any help, thanks! @WikiRik@tomaspanek

@WikiRik

Copy link
Copy Markdown
Member

Initially I thought it was not due to this PR since it didn't touch eslint dependency but other PRs are not failing so maybe the inclusion of timezone_mock did break it. Will take a look at it tonight

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@WikiRik@profnandaa Any chance you could just re-run these checks, please? Since this package is distributed without lockfiles, it is possible that a minor version of a dependency (perhaps babel) had a glitch causing incomplete compilation for Node 6.

The test was failing even before introducing the timezone-mock package; you also can find the same Node 6 issues within checks for other PRs from around the same time.

@profnandaa

profnandaa commented Aug 17, 2023 via email

Copy link
Copy Markdown
Member

@WikiRik

Copy link
Copy Markdown
Member

Indeed seems to be a flaky test since it passes now

@profnandaa

profnandaa commented Aug 18, 2023 via email

Copy link
Copy Markdown
Member

@profnandaa
profnandaa merged commit 2c4aede into validatorjs:masterAug 18, 2023
@profnandaaprofnandaa mentioned this pull request Aug 18, 2023
@sacummings91

Copy link
Copy Markdown

Does anyone know what's going on with this? Seems like it was supposed to be released months ago. We haven't been able to update validator to the latest because of this issue

@manojmula

Copy link
Copy Markdown

@sacummings91 can you please explain me what is the failing scenario for you so I can help you out with.

@sacummings91

sacummings91 commented Dec 1, 2023

Copy link
Copy Markdown

To be completely honest it's been so long now I can't remember the details exactly. I believe this comparison was off by 1 but I can't remember how I was able to surface it

new Date(`${fullYear}-${dateObj.m}-${dateObj.d}`).getDate() === +dateObj.d;

@tomaspanek

Copy link
Copy Markdown
ContributorAuthor

@sacummings91 the issue was caused by the getDate() method returning the day in UTC, which could differ from the actual date being parsed if the runtime environment has a timezone to the west of UTC+0.

This PR has been merged, but it has not been released as part of a new version yet, see #2269. The package maintainers faced some challenges preparing a new build way back in August – given the delay, I assume things just have been very busy on their end.

Unless you need any new features/fixes introduced in the 13.11.0 version, I'd recommend using the 13.9.0 version. In case you have the validator package as a dependency of a dependency, you can include the following in your package.json:

If using yarn:

{
"resolutions": {
"validator": "13.9.0"
}
}

If using npm:

{
"overrides": {
"validator": "13.9.0"
}
}

Hopefully the new release will be out sometime soon!

@sacummings91

Copy link
Copy Markdown

Hey @tomaspanek, thanks for the update. I was mainly curious why it never got updated because I remember I saw it was about to be updated when I was originally dealing with this issue back in August. We have in fact pinned our version to 13.9.0 and aren't facing any issues with that version. I was just checking in to see when it might be safe to update to a newer version and noticed it still hasn't been updated.

Sign up for freeto 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

@tomaspanek@brunoabude@WikiRik@profnandaa@sacummings91@manojmula@rubiin