Switch to classes and ES6/esmodule TS compilation #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@taylorthurlow Thanks for your effort. Your concerns are right but I hope it won't go waste. |
harpreetkhalsagtbit
changed the base branch from
master
to
feature/refactoring-new-typescript-classes
|
@taylorthurlow updated the base branch - I will review and test it further. |
This reverts commit 8a53256.
Also eliminate an unnecessary Country import.
|
Thanks for taking a look! I just rewrote the tests and pushed everything up. I have 100% coverage but there is some weirdness with the branch coverage percentage on |
harpreetkhalsagtbit
changed the base branch from
feature/refactoring-new-typescript-classes
to
develop
|
@taylorthurlow your code is already refactored and merged into the develop branch. |
Thanks! Sorry I've been busy. Will get to this sometime next week I think. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not 100% sure if this PR is welcome - this is obviously a big rewrite/refactor of how this is all structured, and will break every existing app that uses this library. So, no hard feelings if this gets closed, I mostly did this because I'm using this in a web app, and the size of the gzipped bundle when this library is included is WAY too high (2.6mb gzipped).
This is a rewrite which moves the primary Country, State, and City interfaces into their own classes. Each class has appropriate class and instance methods which replicate the existing functionality of the library, but in a way which allows full tree-shaking when one or more classes are not used in the dependent application.
In order to support tree-shaking, I had to change the TypeScript compiler options to generate ES6 compatible modules rather than CommonJS modules, and I'm not really an expert on JS in general, so I don't know what the implications are of that change as far as compatibility. I know that this should be fine for web applications, and I believe newer versions of Node will be fine with ES modules.
I'm still playing around with this, and I haven't updated the tests yet, so I'm marking this as a draft. Let me know if there is any interest in merging this.
Thanks!