[FIX] getCities

[hyanmandian]

Today getCities return just city name, but in Brazil, we have some cities with same name in different states... So I think that we should return stateCode and maybe code (IBGE id) in the returned data.

Something like that:

[
  {
    code: 14324,
    name: 'Bom Jesus',
    stateCode: 'SC',
  },
  {
    code: 14324,
    name: 'Bom Jesus',
    stateCode: 'RS',
  },
  {
    code: 14324,
    name: 'Bom Jesus',
    stateCode: 'RN',
  },
  {
    code: 14324,
    name: 'Bom Jesus',
    stateCode: 'PB',
  },
  {
    code: 14324,
    name: 'Bom Jesus',
    stateCode: 'PI',
  }
]

I know this change will break our API, but since we're still in RC, maybe it's not a big problem. What do you think about it @arantespp @andreoav ?

[andreoav]

@hyanmandian RC should not introduce new breaking changes. I think it is better to deprecate the current API and introduce the new API as another helper.

[hyanmandian]

Maybe we should remove this utility... Talking to @andreoav we discover that this function adds a lot of unnecessary bytes and this type of utility should have an API to consume. We concluded that this is out of our scope.

[arantespp]

Good point about the cities with the same name, but with a different state. I didn't consider this because I've never seen an usage of all cities together because it returns a huge amount of data. The application gets the state first and after the cities of the selected state.

About

Maybe we should remove this utility...

are you talking about removing getCities @hyanmandian?

[hyanmandian]

Yes @arantespp :( This util increase a lot of bytes in our library.

Look that: https://bundlephobia.com/result?p=@brazilian-utils/brazilian-utils@1.0.0-rc.6

Jumps from 18.3kb to 102.4kb

[arantespp]

I see. Is the size huge even with tree shaking? About the project scope, if getCities not fit, I agree that the best is to remove it.

Make sense for the project to create a package called @brazilian-utils/cities?

[hyanmandian]

Yes, even with tree shaking we have a problem because today our tree shaking is not so good :(

I think that we have 2 ways to fix that, the first and easier to do is just remove that function, another way is fixing our tree shaking and maybe create a babel-plugin to remove from our users the knowledge of how to handle with that, for example:

Today we import something like that: import { isValidCPF } from" @ brazilian-utils/brazilian-utils" in a tree shakable way we import something like this: import isValidCPF from" @brazilian-utils/brazilian-utils/is-valid-cpf ";, so the babel plugin will help just replacing @brazilian-utils / brazilian-utils to @brazilian-utils/brazilian-utils. You can see something like that in date-fns or lodash libraries.

I think the second approach is better for our future, but in that case, I don't know if it's applicable. Interacting with IBGE API I discovered that the names of cities changes more frequently then I expect (seriously, every new build that I've generated the cities JSON was updated) and for things like this that are is so variable is better to use an API to consume that. Maybe this function could make a part of BrasilAPI (I saw some issues related to IBGE API).

[arantespp]

I didn't know that the cities name changed a lot. So it's better remove getCities by now, because it fits better with BrasilAPI.

[pedrovsp]

Should another issue be opened regarding the tree-shaking problem? This way we can proceed removing getCities.

[pedrovsp]

Also removing the getCities() method would be a breaking change. Should it be deprecated first and then removed in a next major release?

[hyanmandian]

Nice idea @pedrovsp ! First, we need to deprecate this function with // @deprecated annotation with the reason and maybe provide some alternative (like @brazilian-utils/getCities or something like that). After that, we can remove and increase the version.

Are you guys ok with that?

[pedrovsp]

Yes, sounds good to me! Should another issue be opened regarding the creation of @brazilian-utils/getCities? Will it be a new repo inside the brazilian-utils organization?

[hexetia]

I agree with moving cities to another package, because that will make the core utils much smaller when someone looks in bundlephobia

The "size" ins't really an issue, as I pointed in #157 (comment), but the majority of users will feel afraid after seeing the size of the whole package

and the name maybe something like @brazilian-utils/cities will be better, maybe new utilities related to cities appear, what do you think?

[hyanmandian]

Nice thoughts @saculbr ! I prefer @brazilian-utils/cities, as you said, makes more sense, but since tree shaking works fine like you comments in #157 , I don't know if we need to spend more time discussing if we remove or not this function (because we can do it after some feedback from our users). For me, is better to just can fix the return as I mentioned in this issue.

What do you guys think about that?

[arantespp]

@hyanmandian how is the status of this issue? First will we deprecated the API and after creating @brazilian-utils/cities if code and stateCode?

[hyanmandian]

We just need to decide what we should do. At first, I thought that deprecate this function and create another package make sense, but after @morhogg discovered in #157 that the tree shake works fine, I think we can just improve the current behavior to what I've suggested in this issue and finally publish v1 with a note in the changelog explaning about the API changes.

[arantespp]

can just improve the current behavior to what I've suggested in this issue

Does this mean that we change the getCities return values instead of creating a getCitiesV2?

[hyanmandian]

Yes.. What do you think about it?

[hyanmandian]

Or we can do what @andreoav suggested. I know that this approach it's correct, but I would not like to do that :(

[arantespp]

I prefer keeping getCities, but I don't know the strict rules of a package in an RC. If this package wasn't in RC, I'd agree with @andreoav.

@hyanmandian have you ever considered creating a method with a name like getDetailedCities?

[hyanmandian]

Because today getCities (without the state) returns the wrong data and can cause issues. For example, in the current project that I work on, we replace getCities with a call for internal API because we need more data. I know that getCities(state) (with the state) return the right data, but I think that this behavior is so confusing and makes trouble in the future.

[arantespp]

Make sense. I'd go for keeping the getCities name.

[hyanmandian]

@arantespp can you help us to fix getCities? (I can do that, but right now I'm entering on a little vacation and I prefer to stay away from any code during that time...)

[arantespp]

I'll do that and enjoy your vacation :)

[Isaius]

I don't know if this still up or if its already done, since its from last year. I think its a good idea. So, what's the actual situation?

[hyanmandian]

Hello @Isaius sorry for the delay :( . Right now I don't have much time to focus on current issues. If you know how to improve that, please, be comfortable contributing.