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 broken timezone abbreviations and GMT fallback #166

Merged

Conversation

mikegreiling
Copy link
Contributor

@mikegreiling mikegreiling commented Sep 20, 2021

This PR aims to fix what both #104 and #165 attempted to do, this time with tests to ensure it works as expected and doesn't break in the future.

The gist is this:

The Z formatting option attempts to generate a nice timezone abbreviation by parsing the rendered date string, recognizing something like "Eastern Daylight Time" and changing that to EDT. For all unrecognized timezones (pretty much everything but common North American timezones) it would parse and display the GMT+xxxx offset from the date string instead.

This, however caused a couple of issues with Australian timezones. Because Australia has defined an "Australian Eastern Daylight Time" (for instance) it would erroneously match the same regex as the American "Eastern Daylight Time" and it would render as EST instead of AEST. Adding explicit support for the "Australian " prefix fixes this issue.

The GMT+xxxx fallback worked prior to #165 but it was broken, probably accidentally while rebasing #104 in an attempt to get it into a merge-able state. This PR fixes the issue introduced by #165 and adds some tests to ensure it doesn't get inadvertently broken in the future.

/cc @chase-manning

@mikegreiling mikegreiling mentioned this pull request Sep 20, 2021
@chase-manning
Copy link
Collaborator

@mikegreiling Thanks Mike!!! Appreciate you checking everything after the merge and fixing the issue 😄

@chase-manning chase-manning merged commit 4d26455 into felixge:master Sep 21, 2021
@mikegreiling
Copy link
Contributor Author

@chase-manning any chance I could bother you to publish a new version with these changes? I'd really like to be able to close this bug report over at GitLab 😄

@chase-manning
Copy link
Collaborator

@mikegreiling Absolutely! I'll do that now 😄

Did you get this error when running tests?

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:727:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:46:40)
    at link (internal/modules/esm/module_job.js:45:36)

Getting it now, tried a reclone and everything and still have it.

Really need to setup a yml pipeline to run tests for PRs lol!

@mikegreiling
Copy link
Contributor Author

mikegreiling commented Sep 21, 2021

Did you get this error when running tests?

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:727:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:46:40)
    at link (internal/modules/esm/module_job.js:45:36)

Getting it now, tried a reclone and everything and still have it.

Really need to setup a yml pipeline to run tests for PRs lol!

Yeah I did. I believe I got around it by switching node versions. There were still a lot of local test failures due to #41, but I just focused on the single test file added in this PR and figured as long as the rest still passed in CI, all was fine 😄

@chase-manning
Copy link
Collaborator

Did you get this error when running tests?

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:727:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:46:40)
    at link (internal/modules/esm/module_job.js:45:36)

Getting it now, tried a reclone and everything and still have it.
Really need to setup a yml pipeline to run tests for PRs lol!

Yeah I did. I believe I got around it by switching node versions. There were still a lot of local test failures due to #41, but I just focused on the single test file added in this PR and figured as long as the rest still passed in CI, all was fine 😄

Oh awesome! Updating node worked for me 😄

@chase-manning
Copy link
Collaborator

@mikegreiling
Copy link
Contributor Author

Thank you sir, very much appreciated!

@chase-manning
Copy link
Collaborator

@mikegreiling Changed version to 5.0.1 as realised the recently deployed version introduced breaking changes:
https://github.com/felixge/node-dateformat/releases/tag/v5.0.1
You might want to change to the new import format and then update to 5.0.1 😄

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.

None yet

2 participants