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

Switch to typescript #12

Merged
merged 3 commits into from
May 14, 2019
Merged

Switch to typescript #12

merged 3 commits into from
May 14, 2019

Conversation

harpreetkhalsagtbit
Copy link
Owner

No description provided.

Copy link

@baywet baywet left a comment

Choose a reason for hiding this comment

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

good first iteration, I think we can improve it a bit further

__test__/index.test.js Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@harpreetkhalsagtbit
Copy link
Owner Author

@baywet
Please review the updates.

@harpreetkhalsagtbit
Copy link
Owner Author

harpreetkhalsagtbit commented May 11, 2019

when I locally install my package, I can see dist, lib, __test__ etc all folders in node_modules, even after I have put lib and node_modules in .npmignore file. How can I fix it?

@baywet
Copy link

baywet commented May 14, 2019

the behavior of the npm publish command is that it well take everything located in the current folder.
If you want to exclude anything, it will look at the gitignore file if no npmignore file exists otherwise it will only look at the npmignore file.
I suggest you add all the things you want to exclude from your package in your npmignore:

  • lib
  • index.ts
  • jest.config.js
  • node_modules
  • test
  • lib/test
    (on top of what you already have there)
    I hope this helps

@baywet
Copy link

baywet commented May 14, 2019

(the test folder name was caught by md, i meant _ _ test _ _ without the spaces)

@harpreetkhalsagtbit
Copy link
Owner Author

harpreetkhalsagtbit commented May 14, 2019

the behavior of the npm publish command is that it well take everything located in the current folder.
If you want to exclude anything, it will look at the gitignore file if no npmignore file exists otherwise it will only look at the npmignore file.
I suggest you add all the things you want to exclude from your package in your npmignore:

  • lib
  • index.ts
  • jest.config.js
  • node_modules
  • test
  • lib/test
    (on top of what you already have there)
    I hope this helps

I did add lib and node_modules in .npmignore file. I haven't published it yet. I was doing local install using

npm install ./localpath

This was adding all the folders to respective node_modules, I wonder it would do same when I run npm publish.

Also, please let me know if this P.R is readyToMerge?

@baywet
Copy link

baywet commented May 14, 2019

besides the additional exclusions I was mentioning in my previous comment, it looks good to me. Great work!

@harpreetkhalsagtbit
Copy link
Owner Author

Thanks @baywet
It was really a great help. Thanks for all the efforts. Please be in touch for all the future development/improvements/suggestions. I would like to add you as a maintainer of this repository if you want.

@harpreetkhalsagtbit harpreetkhalsagtbit merged commit da1345d into master May 14, 2019
@baywet
Copy link

baywet commented May 14, 2019

Thank you! Great team effort! And no, please don't add me as maintainer, I don't have the time to maintain this additional project on a regular basis.

@harpreetkhalsagtbit
Copy link
Owner Author

@baywet
after build main index.js returns module as
exports.default instead of module.exports`

Unable to require package and use directly, instead need to call functions using

 var csc = require('country-state-city')
 console.log(csc.default.getAllCountries())

it should be

 var csc = require('country-state-city')
 console.log(csc.getAllCountries())

@baywet
Copy link

baywet commented May 14, 2019

hum I think that's because you have other declarations in your files outside of the default one. So it gets transpiled down to a module, containing a default one a a few other ones.
Try to move the two functions inside the export default and see if this changes anything. Not sure it's not going to push a different problem on the functions instead though.

@harpreetkhalsagtbit
Copy link
Owner Author

export = csc
it got fixed by changing
export default csc
to
export = csc

But I am not sure whether it will work with import

@harpreetkhalsagtbit
Copy link
Owner Author

harpreetkhalsagtbit commented May 14, 2019

Will need to check it thorough, for now I have reverted npm release. Will spend sometime figuring out this issue and will fix it for both require and import if possible

@harpreetkhalsagtbit
Copy link
Owner Author

harpreetkhalsagtbit commented May 15, 2019

@baywet
I am going with

export = {
         getCountries() {}
} 

I have tested it with require() without using '.defaultand normally withimport` as well. Its working
Published to npm as well.

@baywet
Copy link

baywet commented May 23, 2019

Just as a follow up, today I upgraded some of my production projects to this latest version.
Works well and is definitively an improvement over the previous version.

However I have an additional suggestion, today methods have an implicit return type or implicit any. If you declared interfaces for country and state, that'd allow consumers to leverage those interfaces in their code and provide even better type check.

interface ICountry {
    id: string;
    name: string;
    phonecode: string;
    sortname: string;
}
interface IState {
    id: string;
    name: string;
    country_id: string;
}
interface ICity {
   id: string;
   name: string;
   state_id: string;
}

And then all you should need to do is type the return of the methods like this

getCountryById: function (id: string) : ICountry {
		return _findEntry(countryList, id);
	},

@harpreetkhalsagtbit
Copy link
Owner Author

@baywet
please review
#15

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

2 participants