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

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 commented Feb 6, 2017

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

@BenjaminVanRyseghem
Copy link
Owner

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

@olmobrutall
Copy link
Contributor Author

@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 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

@BenjaminVanRyseghem, something missing for this to get merged?

@BenjaminVanRyseghem
Copy link
Owner

@olmobrutall me missing time 😄

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



Choose a reason for hiding this comment

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

one empty line feels enough 😄

@olmobrutall
Copy link
Contributor Author

fixed :)

@olmobrutall
Copy link
Contributor Author

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

@morlay
Copy link

morlay commented Feb 18, 2017

Could we release a patch for this?

@BenjaminVanRyseghem
Copy link
Owner

@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 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

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

@roni-frantchi
Copy link

@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

@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 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

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

@olmobrutall
Copy link
Contributor Author

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 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

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

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
@bherila
Copy link

bherila commented Apr 25, 2017

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

@BenjaminVanRyseghem
Copy link
Owner

@bherila no clue 😄

@schmuli
Copy link

schmuli commented Apr 26, 2017

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

import * as numbro from 'numbro';

@bherila
Copy link

bherila commented Apr 26, 2017

It worked @schmuli , thanks!

@BenjaminVanRyseghem
Copy link
Owner

in numbro 1.11.0

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

7 participants