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

typings - global Bignumber #143

Closed
olaf89 opened this issue Feb 7, 2018 · 14 comments
Closed

typings - global Bignumber #143

olaf89 opened this issue Feb 7, 2018 · 14 comments

Comments

@olaf89
Copy link

olaf89 commented Feb 7, 2018

/**
 * Browsers.
 */
declare global {
  const BigNumber: BigNumberConstructor;
  type BigNumber = BigNumberInstance;

  namespace BigNumber {
    ...
  }
}

Why bignumber is this defined as global? I think this declaration should be removed cause its really error prone.

@MikeMcl
Copy link
Owner

MikeMcl commented Feb 7, 2018

Please explain what you mean.
The code is necessary to use the typings outside a module loader environment.
This library augments the global namespace with BigNumber if used in a browser, and the typings reflect that.

@olaf89
Copy link
Author

olaf89 commented Feb 7, 2018

I do use bigNumber in the browser, but i import it using es6 named imports

import { BigNumber } from 'bignumber.js' 

But now when i skip this import, typescipt does not report any issues and its failing during runtime, cause BigNumber is not found.

@MikeMcl
Copy link
Owner

MikeMcl commented Feb 7, 2018

BigNumber would only not be found in a browser if the bignumber.js script isn't being loaded.
If you're not loading the script then why include the typings?

@olaf89
Copy link
Author

olaf89 commented Feb 7, 2018

Ok, the thing is that for projects in node.js & projects using module loaders including bigNumber as global object in typings is highly error prone and harms developer experience - cause you cannot use auto import.

@MikeMcl
Copy link
Owner

MikeMcl commented Feb 7, 2018

Please explain further.

Do you mean: because BigNumber is declared global there is no intellisense about the missing import and option to automatically import it?

If so, then I see what you mean. But if I take out the declare global it means having to have two separate declaration files, one for modules, one for global.

@olaf89
Copy link
Author

olaf89 commented Feb 7, 2018

Well its very simple, i create simple file on my project (using webpack and bignumber 6.0.0)

screen shot 2018-02-08 at 00 31 58

Its looks all good typescript is happy, then i run it and it fails.

Lets compare it to version 5.0.0 of bignumber without global export
screen shot 2018-02-08 at 00 36 47

It reports it as an error and wont let me compile the project.

So as you can see safety is lost, additionally possibility to quickly fix that issue will never show cause its not detected as an issue

@MikeMcl
Copy link
Owner

MikeMcl commented Feb 8, 2018

Yes, so it's a matter of being able to catch the missing import at compile time or with intellisense.

But if I remove the declare global section and create separation declaration files for module and global usage, then that opens a can of worms regarding how easy it is for users to configure the typescript compiler to find the particular declaration file they need.

I suppose I would have the types property of this library's package.json pointing to the declaration file for module usage, as that is probably wanted more often, but then that may make it difficult for users wanting the other declaration file for global usage.

I'll consider it further.

@pgebheim
Copy link

pgebheim commented Mar 27, 2018

This definition creates another (major) issue:

The Scenario:

  • Project P depends on BigNumber6
  • Module M depends on BigNumber6, and exports typings that use BigNumber
  • Module M is npm link or yarn linkd into Project P

IN this case, there are two copies of bignumber.d.ts in the source tree, due to the symlink used by npm link.

P/node_modules/bignumber.js/bignumber.d.ts
P/node_modules/M/node_modules/bignumber.d.ts

tsc will then process this typings file as part of the typings for module M and again when project P imports BigNumber.js

In this case, we get an ERROR from tsc because of the global declarations because we've globally exported n object not a module:

node_modules/bignumber.js/bignumber.d.ts(1760,9): error TS2649: Cannot augment module 'BigNumber' with value exports because it resolves to a non-module entity.

The core reason you're doing this is to make sure that the typings are available for usage when the actual JS is used in a non-module environment. This doesn't make a lot of sense since if they're using Typescript to get the typings they are by definition in a module-supporting environment. And furthermore, if they're NOT then the typings are totally ignored in any case.

So I don't see what problem it solves to have this global export accomplished in this particular way, and it is clearly causing problems.

@MikeMcl
Copy link
Owner

MikeMcl commented Mar 27, 2018

@pgebheim

I can't make complete sense of your post, but, yes, I agree that it would be better to remove the global stuff from the definitions file, as I did recently with decimal.js.

I'll get on it presently.

@lanthaler
Copy link

+1, this is creating issues for me too

@thehappycoder
Copy link

thehappycoder commented Apr 10, 2018

I get the same error as @pgebheim

As a workaround, for now, export the BigNumber in my package:

export { BigNumber } from 'bignumber.js'

and use it in my project instead of the one imported directly from 'bignumber.js'

@MikeMcl
Copy link
Owner

MikeMcl commented Apr 11, 2018

This doesn't make a lot of sense since if they're using Typescript to get the typings they are by definition in a module-supporting environment. And furthermore, if they're NOT then the typings are totally ignored in any case.

Total nonsense. But anyway, I'll release a new version without the global typings at the end of next week.

@MikeMcl
Copy link
Owner

MikeMcl commented Apr 26, 2018

Removed global BigNumber from typings in v7.0.0.

@MikeMcl MikeMcl closed this as completed Apr 26, 2018
@dmichael
Copy link

dmichael commented May 9, 2018

Thanks for correcting this! I just hit the situation where TS compiled happily, but at runtime BigNumber was missing. The update to v7 did the trick.

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

No branches or pull requests

6 participants