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

imp: Migrate platform-ios to TypeScript #620

Merged
merged 8 commits into from
Sep 9, 2019
Merged

imp: Migrate platform-ios to TypeScript #620

merged 8 commits into from
Sep 9, 2019

Conversation

Esemesek
Copy link
Member

@Esemesek Esemesek commented Aug 5, 2019

Summary:

This PR includes moving the whole platform-ios module to Typescript. There were a lot of issues while moving this module because most of it was untyped and the main library that we rely on (xcode) is untyped. Also, there is a lot of explicit casting to any type, empty stub definitions for xcode module and plenty of @ts-ignore.

Changes:

  • Rename .js to .ts
  • Fixed all the Typescript issues in platform-ios
  • New module for all the common types cli-types

TODO:
- [ ] - Fix as many types issues as possible
- [ ] - Improve typings for xcode

^ Since those will take much more effort and time to make, I think we can make issues for them after we merge this PR.

Test Plan:

👍- All the check are passing.

@Esemesek Esemesek changed the title imp: Migrate platform-ios to TypesScript imp: Migrate platform-ios to TypeScript Aug 11, 2019
@Esemesek Esemesek marked this pull request as ready for review August 22, 2019 16:06
packages/platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved
@@ -18,18 +16,21 @@ describe('pods::isInstalled', () => {
it('returns false if pod is missing', () => {
const project = {podfile: path.join(PODFILES_PATH, 'PodfileSimple')};
const podspecName = {podspecPath: '/path/NotExisting'};
// @ts-ignore FIXME: Improve types
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the issue with these?

Copy link
Member Author

Choose a reason for hiding this comment

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

The isInstalled function is not written correctly. We are passing whole big objects to the function, but not all properties are used. I've added ts-ignore in the test to prevent adding unused mock data.

packages/platform-ios/src/link-pods/addPodEntry.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/link/common/isInstalled.ts Outdated Show resolved Hide resolved
packages/platform-ios/src/link/getTargets.ts Outdated Show resolved Hide resolved
export default function removeFromStaticLibraries(
project: any,
path: string,
opts: {[key: string]: any},
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you do a follow up PR to fix all instance of any? There's an awful lot of them…

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the plan. We have so many instances of any because library that we rely on - https://www.npmjs.com/package/xcode - has no typings. I want to write them in the future and make this module better. For now, this PR is big enough and didn't want to include the next big thing.

@Esemesek
Copy link
Member Author

Esemesek commented Sep 9, 2019

@tido64 I resolved all the comments that were fixed. There is still plenty of work to do. We have to add types to the xcode module and then make types more strict. Thank you for walking through this quite long and boring PR C:

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looking good already. Let's tackle anys in a followup. Feel free to ask community for some help :)

@thymikee thymikee merged commit 4870ac5 into master Sep 9, 2019
@thymikee thymikee deleted the ios-typescript branch September 9, 2019 17:05
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.

3 participants