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

Fix TS type declarations for BasemapName and CartoSlice #248

Merged
merged 5 commits into from
Dec 28, 2021

Conversation

juandjara
Copy link
Contributor

@juandjara juandjara commented Dec 16, 2021

Hello team! I've found some Typescript errors when setting up a new project from the template with Carto3 and Typescript and I saw a very easy correction of them so I created this PR. Let me know if you need any more context, tests, docs or anything like that

There are two changes in this PR:

  • changing the type of the createCartoSlice function. Instead of using the custom Reducer type from the types.ts file of the C4R library, we are importing the Reducer type from redux. This is a generic type, and as such, it should be defined as Reducer<any, AnyAction>. The VS Code tooltips let me know about this when I saw that the types for adding the cartoSlice to the store in the TS-template were not matching the redux types.

  • changing the type of BasemapName from using & to using |. The & operator in Typescript creates an intersection between types that does not work for joining two enum types together. I learned about that here when I was debugging why VS Code was again complaining in the basemap references scattered across the template. After much research, try, and error, I discovered that you just need to change the & for an | to mean "This type represents values from this enum OR this other enum"

Edit:
Following @Clebal comment and the redux-typescript documentation I've changed the cartoSlice type to further indicate the types of the reducer using Reducer<CartoState, AnyAction>

@coveralls
Copy link
Collaborator

coveralls commented Dec 16, 2021

Pull Request Test Coverage Report for Build 1591994246

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 70.422%

Totals Coverage Status
Change from base Build 1583593077: 0.0%
Covered Lines: 963
Relevant Lines: 1278

💛 - Coveralls

@juandjara juandjara changed the title fix TS type declarations for BasemapName and CartoSlice Fix TS type declarations for BasemapName and CartoSlice Dec 17, 2021
@VictorVelarde
Copy link
Contributor

Code looks good to me, but we should check against Builder, linked locally, before we merge

Copy link
Contributor

@Clebal Clebal left a comment

Choose a reason for hiding this comment

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

@juandjara great job! 🚀

@Clebal
Copy link
Contributor

Clebal commented Dec 17, 2021

@VictorVelarde it's ready to check agains builder.

@juandjara
Copy link
Contributor Author

@Clebal @VictorVelarde thanks for the quick response!
Just FYI, I've checked in my project (Initiative-Vision) and also ran yarn build in the root folder carto-react and everything works fine 👌

@alasarr alasarr merged commit c924228 into master Dec 28, 2021
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.

6 participants