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

dep types: Use TsFlower for expo-application and expo-screen-orientation #5447

Merged
merged 2 commits into from
Jul 23, 2022

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Jul 22, 2022

I'd like to do this on the way to #5441, upgrading to Expo SDK 44.

This would have interfered with types/expo-*/build/, which we'll add
soon using tools/tsflower.
@chrisbobbe chrisbobbe requested a review from gnprice July 22, 2022 02:25
@chrisbobbe chrisbobbe marked this pull request as ready for review July 22, 2022 02:25
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @chrisbobbe! This looks good -- small comments below.

Comment on lines +154 to +156
package=expo-screen-orientation
run_on_package "${package}"
format_dir "${rootdir}"/types/"${package}"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's still a file flow-typed/expo-screen-orientation_vx.x.x.js, so we should delete that.

Comment on lines +61 to +71
+declare export var ApplicationReleaseType: {|
+ +UNKNOWN: 0,
+ +SIMULATOR: 1,
+ +ENTERPRISE: 2,
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to mention that this doesn't describe all the quirky behavior of these TS enums (namely that it maps values to names as well as names to values.)

@gnprice
Copy link
Member

gnprice commented Jul 22, 2022

Oh also: the references to ft-flow in eslint-ignore lines look like they depend on part of #5444 .

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jul 22, 2022

Thanks for the review! Revision pushed. I removed the ft-flow eslint-ignore lines; it seems tools/test lint doesn't cover the files in types/ anyway. (Not sure why, or maybe I'm missing some nuance there.)

@gnprice
Copy link
Member

gnprice commented Jul 23, 2022

Thanks! Looks good; merging.

@gnprice gnprice merged commit cfa0fdf into zulip:main Jul 23, 2022
@chrisbobbe
Copy link
Contributor Author

Thanks for the review!

@chrisbobbe chrisbobbe deleted the pr-expo-tsflower branch July 23, 2022 00:08
@gnprice
Copy link
Member

gnprice commented Jul 23, 2022

it seems tools/test lint doesn't cover the files in types/ anyway. (Not sure why, or maybe I'm missing some nuance there.)

Yeah, that's ultimately due to this bit:

# Intersect $files with the set of our JS files in src/.
#
# Prints a list of newline-terminated paths; either files, or
# directories meaning their whole subtrees.
files_js() {
    local file_pattern='^src/.*\.js$'

If we want to lint the type definitions, that could be accomplished by changing the lint case here to be like the prettier case:

        lint)
            run_lint $(files_js)
            ;;
        jest)
            run_jest
            ;;
        prettier)
            run_prettier $(files_js_jsflow)
            ;;

as files_js_jsflow is defined to apply to those too.

I'm not sure we do want lint on the type definitions, though, or at least the generated ones. When I open one of the longer files in types/@react-navigation/, it seems like there are a lot of lint errors that wouldn't be productive to try to resolve.

@chrisbobbe chrisbobbe modified the milestone: py Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants