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

Make strftime play nice with dates that have been extended by DateJS #69

Merged
merged 1 commit into from
Dec 27, 2016

Conversation

stiang
Copy link
Contributor

@stiang stiang commented Dec 31, 2015

A project of mine uses DateJS (which may or may not be a good idea), and it causes strftime to not use the default locale (and thereby throw an error unless you have supplied a locale yourself) because the date argument will have a days key, making strftime believe that it is in fact a locale argument.

I added a simple check to make sure the argument does not have a typical date key (getTime), which should never be present in a locale (AFAICT). All tests pass.

@alexandrnikitin
Copy link
Collaborator

Hi Stian. Thanks for the PR. A couple of questions: Why don't you use the new API described here https://github.com/samsonjs/strftime#usage What about checking locale argument instead: if not defined then d is a locale...

@stiang
Copy link
Contributor Author

stiang commented Jan 1, 2016

Hi,

I am using the new API. Here’s a bit of code that demonstrates the problem:

var strftime = require('strftime');

// Works fine
console.log(strftime('%F %T', new Date()));

require('datejs');

// No longer works
console.log(strftime('%F %T', new Date()));

Here’s the output from running the above:

❯ node index.js
2016-01-01 22:14:05
[WARNING] `require('strftime')(format, [date], [locale])` is deprecated and will be removed in version 1.0. Instead, use `var s = require('strftime').localize(locale); s(format, [date])`.

/foo/strftime-test/node_modules/strftime/strftime.js:257
                       resultString += _processFormat(locale.formats.F, date,
                                                                    ^
TypeError: Cannot read property 'F' of undefined
    at _processFormat (/foo/strftime-test/node_modules/strftime/strftime.js:257:74)
    at _strftime (/foo/strftime-test/node_modules/strftime/strftime.js:185:20)
    at adaptedStrftime (/foo/strftime-test/node_modules/strftime/strftime.js:85:16)
    at Object.<anonymous> (/foo/strftime-test/index.js:7:13)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)

As you can see it mistakenly prints out the deprecation notice, since it believes that the date argument is in fact a locale definition. With my PR this no longer happens. I’d be happy to check something else than getTime, I just used the first thing that seemed safe and sensible :)

@samsonjs samsonjs merged commit 65aa4d5 into samsonjs:master Dec 27, 2016
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.

3 participants