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

update numbro.d.ts to use export = instead of export default #232

Merged

Conversation

@olmobrutall
Copy link
Contributor

@olmobrutall olmobrutall commented Feb 5, 2017

As https://github.com/foretagsplatsen/numbro/blob/develop/dist/numbro.js shows the module is exported as a namespace (module.exports = numbro;), but the Typescript .d.ts file was exporting it using export default numbro.

This discrepancy makes it impossible to use in TS:

  • If imported using `import numbro from "numbro", then numbro is undefined at runtime.
  • If imported using `import * as numbro from "numbro", TS finds no member.

This commit changes numbro.d.ts to use exports = that is the correct way of representing the current javascript behaviour.

@smajl
Copy link

@smajl smajl commented Feb 6, 2017

Hm, strange, import numbro from "numbro"; works in our project with no problem.

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Feb 6, 2017

I don't use TS myself, so I can't really help, sorry 😕

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Feb 6, 2017

@smajl, what module loader are you using? I know that babel and SystemJS do some magic vodoo to hide the differences but this 'feature' is being reconsidered as mentioned here: microsoft/TypeScript#5565

@BenjaminVanRyseghem, in general in ES2016:

  • If export.default = numbro then import numbro from "numbro"
  • If export = numbro then import * as numbro from "numbro"

As I said some loaders/bundlers hack it arround to make things simpler, but actually they make it more confusing...

TS is strict about it an, since numbro.js is not using default, then export = is the right approach.

It works in my machine (TM) and is the same that moment.d.ts is doing: https://github.com/moment/moment/blob/develop/moment.d.ts

@smajl
Copy link

@smajl smajl commented Feb 7, 2017

@olmobrutall We are using JSPM = SystemJS, so that might be the case. Anyway I second your change proposal, it's the correct way. 👍

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Feb 10, 2017

@BenjaminVanRyseghem, something missing for this to get merged?

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Feb 13, 2017

@olmobrutall me missing time 😄

numbro.d.ts Outdated
export function loadLanguagesInNode(): void;



This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 13, 2017
Owner

one empty line feels enough 😄

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Feb 13, 2017

fixed :)

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Feb 17, 2017

C'mon a little bit of attention, I just want to make your library great again (for TS users)

@morlay
Copy link

@morlay morlay commented Feb 18, 2017

Could we release a patch for this?

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Feb 20, 2017

@olmobrutall I would love to have more time dedicated to numbro. If you want to negotiate that with my boss, be my guest 😛

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Feb 20, 2017

We're al in the business of unrecognized and unpayed work in github.

The thing is that numbro is currently unusable for me (and probably like half of the TS users) because of a change that only needs to get merged.

If you think that maintaining the d.ts file is a big burden because there is no contributor using TS and it is seen as an alien technology, just delete the file and let it evolve independently in DefinitelyTyped. They currently don't accept my pull request there (DefinitelyTyped/DefinitelyTyped#14412) because they think the d.ts is maintained here.

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Mar 22, 2017

My pull request was accepted in DefinitelyTyped/DefinitelyTyped#14412 so I close this pull request...

@roni-frantchi
Copy link

@roni-frantchi roni-frantchi commented Mar 23, 2017

@olmobrutall Thanks for this one.
Though now, I now find myself in a situation where I can't use the definitions on DefinitlyTyped (since DefinitelyTyped/DefinitelyTyped#14412 (comment)), whilst this definition is still broken due to the faulty export...

How did you get around that?

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Mar 23, 2017

@roni-frantchi, I moved to numeral.js because parsing problem (parse 100.000 in Europe) and the typescript issues

@roni-frantchi
Copy link

@roni-frantchi roni-frantchi commented Mar 26, 2017

I moved to numeral.js because parsing problem (parse 100.000 in Europe) and the typescript issues

Thanks @olmobrutall .

@schmuli @brachi-wernick maybe we should consider doing the same.

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Mar 27, 2017

@olmobrutall how can I help?
(what parsing problem?)

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Mar 27, 2017

The parsing problem was when trying to do this:

número.culture("de-de")
numbro("100.000").format() // returns "100", instead of "100.000"

The same worked with numeral.js unformat method/constructor, but I didn't found the equivalence in numbro.js docs .

I didn't bothered to add the issue since you looked busy.

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Mar 27, 2017

please add a ticket 😄

I do am busy, but I am not alone, and when I can find some time, it's good to know what to tackle 😄

Edit: I added #261

@olmobrutall
Copy link
Contributor Author

@olmobrutall olmobrutall commented Apr 2, 2017

After the fix of the parse problem (just using unformat), we decided to give numbro a second chance.

Any change of this PR to be merged?

We have our own copy of the d.ts file so we can live without the merge, but I'm quite sure the current implementation is wrong and will create problems by any other developers using TS. It's also the same that moment or numeral are doing...

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Apr 3, 2017

I fine with merging it 😄

But for the record, I have no idea what it's doing 😛

@BenjaminVanRyseghem BenjaminVanRyseghem merged commit fb56d55 into BenjaminVanRyseghem:develop Apr 3, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bherila
Copy link

@bherila bherila commented Apr 25, 2017

So what is the proper way to import numbro using typescript now?

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Apr 26, 2017

@bherila no clue 😄

@schmuli
Copy link

@schmuli schmuli commented Apr 26, 2017

@bherila You should be able to use the * as syntax:

import * as numbro from 'numbro';
@bherila
Copy link

@bherila bherila commented Apr 26, 2017

It worked @schmuli , thanks!

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented May 2, 2017

in numbro 1.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.