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-web-browser (and expo-modules-core) #5449

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

chrisbobbe
Copy link
Contributor

expo-web-browser because I remembered it's one of the deps we'd like to upgrade in #5441, and expo-modules-core so that it's available to types/expo-*.

@chrisbobbe
Copy link
Contributor Author

Just rebased, resolving some conflicts. (Kinda tricky this time! 🙂)

As I've used TsFlower for more expo-* libraries, I've regularly had
to suppress unresolved-import errors on imports from
expo-modules-core. I'd expect to have to keep doing that, as long as
we don't have Flow types for expo-modules-core.

So, add some, using TsFlower, and adjust the patches so those
now-unneeded suppressions never get added.
@gnprice gnprice force-pushed the pr-tsflower-expo-web-browser branch from 3e87323 to 27c6742 Compare August 30, 2022 02:57
@gnprice gnprice merged commit 27c6742 into zulip:main Aug 30, 2022
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, modulo one comment below.

... Ha, and then I forgot that one comment and went ahead and merged, oops. Nothing critical, but would be a nice followup.

Comment on lines +19 to +20
+/* $FlowFixMe[cannot-resolve-module]: expo-modules-core should (peer-)depend
+ on react-native-web if it wants this to be a meaningful type. */
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, or perhaps better: we could delete this file entirely, say in tools/tsflower after running TsFlower (in run_only.)

In general we don't ever care about a .web.js.flow file, because it isn't applicable when not on web. In a future where TsFlower has a feature to tell it to skip some files, as suggested here:

    # TODO: Send r-n-tab-view a .npmignore patch to stop shipping this directory.
    #   Also perhaps give TsFlower a way to tell it things to skip.
    rm -rf "${rootdir}"/types/"${package}"/lib/typescript/example/

we'd have entries there like *.web.d.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! #5478

@chrisbobbe chrisbobbe deleted the pr-tsflower-expo-web-browser branch August 30, 2022 22:13
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Aug 30, 2022
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Just sent #5478 for that followup.

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.

2 participants