search

adopted-ember-addons / ember-moment

MIT License
400 stars 122 forks source link

Improve README (especially for Helpers) #271

Closed fenekku closed 6 years ago

fenekku commented 6 years ago

Hello,

I am new to Ember and I was relatively confused by the Helpers documentation of the README. Since I am perhaps not the only one, I took a stab at detailing the README some more.

I discovered a couple of weird things doing so however. I am probably missing something obvious but it would be great if I could be enlightened (so I can further improve the README):

1- Why are locale and timeZone optional named parameter? If I don't have the locale/timeZone in my global configuration, passing them in a helper does not help parsing the string, and if I do have them in my global configuration, then I can parse a string without them. Disambiguation maybe?

2- hideSuffix/hidePrefix would it be worthwhile having a single named parameter that does both instead? hideAffix?

3- If we add suffix/prefix hiding for moment-from/to then moment-from-now/to-now become unnecessary (moment-from/to default to now anyway).

4- Why is this happening with moment and unix?:

5- Should a moment prefix be prepended to all helpers this add-on provides? What would happen if another addon provides a is-before helper? I think this was a pragmatic choice so I am more curious into the trade-off.

6- moment-add, moment-substract, moment-to-date still need to be documented.

7- Why is this happening?: 

{{!-- garbage was not passed into the Handlebars template --}}
{{moment-format garbage}} {{!-- <now> --}}`

8- allow-empty is inconsistent or I still don't understand it:

{{moment ''}}  {{!-- Invalid date --}}
{{moment '' allow-empty=true}} {{!-- Invalid date --}}

Anyways, thanks for the library!

jasonmit commented 6 years ago

Thank you for your PR, really appreciate it.

Why are locale and timeZone optional named parameter? If I don't have the locale/timeZone in my global configuration, passing them in a helper does not help parsing the string, and if I do have them in my global configuration, then I can parse a string without them. Disambiguation maybe?

Unsure what you mean, can you give a quick example. Is the global locale/timeZone is overriding the inlined locale/timeZone?

hideSuffix/hidePrefix would it be worthwhile having a single named parameter that does both instead? hideAffix?

I like this idea, mind submitting a PR or opening an issue for this feature?

allow-empty is inconsistent or I still don't understand it:

{{moment ''}}  {{!-- Invalid date --}}
{{moment '' allow-empty=true}} {{!-- Invalid date --}}

Sounds like a bug. I'll validate this and patch if necessary.

{{!-- garbage was not passed into the Handlebars template --}} {{moment-format garbage}} {{!-- --}}`

I'd expect this to return the equivalent of moment(undefined).format() if allow-empty=true. If that's not the behavior, it's a bug.

fenekku commented 6 years ago

Thanks for merging this! I numbered my questions so it will be easier to talk about them ;).

1- I thought this behaviour was strange:

On the one hand:

// config/environment.js
let ENV = {
  ...
  moment: {
    includeLocales: ['fr']
  }
{{moment-format '25 décembre' 'MM-DD-YYYY' 'DD MMMM'}}    {{!-- 12-25-1995 --}}

On the other, if we comment out includeLocales:

// config/environment.js
let ENV = {
  ...
  //moment: {
  //  includeLocales: ['fr']
  //}
{{moment-format '25 décembre' 'MM-DD-YYYY' 'DD MMMM'}}  {{!-- {{ Invalid Date }} --}}
{{moment-format '25 décembre' 'MM-DD-YYYY' 'DD MMMM' locale='fr'}}  {{!-- {{ Invalid Date }} --}}

So, it seems like we never need locale. What am I missing?

2- I will! However I was wondering how is deprecation typically handled in these projects? I would be in favour of just having the affix form, but we don't want to break existing code. Bumping the major version for this is a bit much. On the other hand can we say that this is a bug fix since even momentjs doesn't do this distinction? We probably want to add the affix form for now, but mark the others as deprecated. Is there a way to do that (communicating deprecation) easily? I could document it, I guess.

Hopefully the other questions were clear.

jasonmit commented 6 years ago

I thought this behaviour was strange

Perhaps with moment().format() providing the locale isn't necessary if it has the locale data to negotiate the locale based on the input string. I'd honestly need to dig through momentjs to determine that and if it's reliable across all locales.

However I was wondering how is deprecation typically handled in these projects? I would be in favour of just having the affix form, but we don't want to break existing code.

I'd say add affix and throw a deprecation warning that prefix/suffix are being removed in favor of affix. Next major release I can remove prefix/suffix.