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

Made CommonJS the first branch of the export if statement #87

Closed
wants to merge 1 commit into from
Closed

Made CommonJS the first branch of the export if statement #87

wants to merge 1 commit into from

Conversation

johnnyreilly
Copy link

@johnnyreilly johnnyreilly commented Sep 27, 2017

Hi!

This recent PR added a named commonjs export for Big. @googol subsequently amended the DefinitelyTyped definitions to cater for this.

Essentially the goal of his changes was to move usage in a more es6 direction; namely making this legitimate:

import { Big } from 'big.js';
const lookABigJs = new Big(1);

That's great, however webpack picks up the first exported branch it can when Big is exported. So it picks up the AMD export not the CommonJS export. You can work around this in webpack by deactivating AMD imports but that's somewhat brute force.

Rather than doing that would you consider flipping the export order as I have done in this PR? So CommonJS is the first branch and AMD is the second. This will resolve issues for Big users of webpack with TypeScript in a hopefully "futurish" fashion.

If you're interested in the context for this then read a really long post here:

microsoft/TypeScript#18791

PS As an aside I updated the version number in line with the package.json.

@googol
Copy link
Contributor

googol commented Sep 27, 2017

Another possibility would be to remove the module.exports.Big = Big line and add Big.Big = Big on line 1130. That way all branches would have the same export shape, and no old functionality would be disturbed.

The AMD - commonjs - global order comes from the UMD templates directly, and breaking the order might confuse others.

@johnnyreilly
Copy link
Author

Sounds good - do you want to submit a PR to replace this one?

@googol
Copy link
Contributor

googol commented Sep 27, 2017

Sure

@MikeMcl
Copy link
Owner

MikeMcl commented Sep 27, 2017

This should be resolved with v4.0.0 which I've just pushed.

@johnnyreilly
Copy link
Author

Thanks! I'll give it a test today

@googol
Copy link
Contributor

googol commented Sep 28, 2017

4.0.0 looks good! I'll make a PR for the typings as some of the config variables changed too.

@johnnyreilly
Copy link
Author

Sweet - thanks! Give me a shout when you raise the pr and I'll review it

@johnnyreilly
Copy link
Author

Typings upgraded here: DefinitelyTyped/DefinitelyTyped#20096 and now released on npm - nice work @MikeMcl and @googol!

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