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

Js to ts #52

Merged
merged 6 commits into from
Sep 22, 2023
Merged

Js to ts #52

merged 6 commits into from
Sep 22, 2023

Conversation

sarahsporck
Copy link
Collaborator

@sarahsporck sarahsporck commented Aug 17, 2023

Description

Fixes #4

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I formatted JS and TS files with running yarn lint:fix in the root folder
  • I have run tests via yarn test in the root folder
  • I updated the documentation with running yarn generate in the root folder
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/updated a sample (/example)

I took quite a bit of inspiration from the https://github.com/rnmapbox/maps repo, as they already "finished" their ts migration a while ago.

add typescript types

rename javascript folder

add Style typings & configure eslint

fixup tests

fix example app

rename folder back to javascript

use geojson types

Use working map url

Cleanup example
@tyrauber
Copy link
Collaborator

@sarahsporck This is awesome! Once the snapshots are updated yarn test -- -u the tests pass. There are a lot of changes here. What needs to happen in this branch before it can be merged? Are you hoping to resolve the ~130 outstanding lint issues? Would you like some help?

@sarahsporck
Copy link
Collaborator Author

So the snapshot tests are updated. regarding the linting issues. I tried not to change to much existing code. And it seems there are at least more than a 100 linting warnings on the main branch already. As this pr is already quite unbearably large I'd tackle them in a separate pr on top of this one :)

Regarding reviewing. I think it makes sense to do more of a functional review, as doing a full fledged code review would eat a lot of time and resources. Although there are a few files where I did struggle a bit during refactoring, which would be the MapView and Camera, as they both contain a lot of code, as well as the animated stuff, as it was not well documented. Maybe you could focus on that :)

@tyrauber
Copy link
Collaborator

Hi @sarahsporck, yeah, I agree. E2E tests pass, so theoretically the changes didn't impact the api or functionality 😂, but I think it is worth build and testing the client manually. I'll go ahead and do that and report back, but I suggest others do that as well. We should probably do a major version bump and release an alpha just to be on the safe side.

@sarahsporck
Copy link
Collaborator Author

Would be nice to get another review here. :)

@tyrauber
Copy link
Collaborator

tyrauber commented Sep 1, 2023

Hi @sarahsporck, tests pass, builds for both Android and iOS on the simulators. Core functionally works. LGTM. Nice work!

@sarahsporck
Copy link
Collaborator Author

yep. Sadly I don't have write access and therefore cannot merge :/

@tyrauber
Copy link
Collaborator

tyrauber commented Sep 4, 2023

@ianthetechie, would it be possible to get this reviewed and merged. It's be nice to do a major bump and release with an alpha tag. I've got a local expo branch that builds upon this. Once this is merged and rebase and push that up for review.

@ianthetechie
Copy link
Collaborator

ianthetechie commented Sep 11, 2023

Thanks @sarahsporck and @tyrauber!

And it seems there are at least more than a 100 linting warnings on the main branch already. As this pr is already quite unbearably large I'd tackle them in a separate pr on top of this one

Agreed.

Regarding reviewing. I think it makes sense to do more of a functional review, as doing a full fledged code review would eat a lot of time and resources.

Also agreed; I'll give a quick skim but focus more on validating that nothing serious broke.

Sadly I don't have write access and therefore cannot merge.

Could we add Sarah as a contributor with write access @wipfli? She has been immensely helpful and I think it makes sense to give her write access.

It's be nice to do a major bump and release with an alpha tag.

Agreed on merge + do a prerelease. Though if the API hasn't changed, I'm not sure it's necessary to do a major bump from a semver perspective. EDIT: It looks like the API has indeed changed some, so this will be a major release.

@wipfli
Copy link

wipfli commented Sep 13, 2023

Yes let's grant @sarahsporck write access to this repository. We usually announce it when people get elevated access rights in the maplibre slack channel. @ianthetechie would you like to do the announcement there?

@wipfli
Copy link

wipfli commented Sep 13, 2023

Added @sarahsporck as write and elevated @ianthetechie to admin. Congrats both and thanks for your contributions!

@ianthetechie
Copy link
Collaborator

ianthetechie commented Sep 13, 2023 via email

@sarahsporck
Copy link
Collaborator Author

So @ianthetechie. I am free to finish this up then? :)

@ianthetechie
Copy link
Collaborator

Yes, this looks good to me code-wise. Feel free to merge once you're happy with this and do an alpha release with a new major version @sarahsporck :D

@sarahsporck sarahsporck merged commit 764a3eb into maplibre:main Sep 22, 2023
@sarahsporck sarahsporck deleted the js-to-ts branch January 24, 2024 17:09
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.

Migrate from JS to TS
4 participants