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

Update AGP to 7.2.2; run yarn upgrade #5818

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

chrisbobbe
Copy link
Contributor

No description provided.

@chrisbobbe chrisbobbe added the dependencies Pull requests that update a dependency file label Jan 23, 2024
@chrisbobbe chrisbobbe requested a review from gnprice January 23, 2024 22:46
@gnprice
Copy link
Member

gnprice commented Jan 24, 2024

Thanks for taking care of this!

All LGTM except it looks like it needs a tools/tsflower run to refresh some of the type definitions.

@chrisbobbe chrisbobbe force-pushed the pr-yarn-upgrade-2024-01-23 branch from a582a3b to c0c9054 Compare January 24, 2024 21:58
@chrisbobbe
Copy link
Contributor Author

Done; thanks! Revision pushed.

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! There's now a later failure in CI:

Run tools/verify-webview-js
generatedEs3.js is not updated with current js file.
Error: Process completed with exit code 1.

(If we weren't in the process of abandoning this codebase, we should probably fold that check into tools/test like the rest of them.)

Also a comment below just about the patch message on one of the new TsFlower patches.

Comment on lines 4 to 9
Subject: [tsflower] rnsac: Fix `React.JSX.Element`s that didn't get rewritten

I think this is supposed to be taken care of by TsFlower...see
prepGlobalJsxRewrites in rewrite/react.ts. Anyway, just do it
manually, using the type that it looks like TsFlower should have
used (`JSX$Element` in subst/react.js.flow).
Copy link
Member

Choose a reason for hiding this comment

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

Hmm fun.

I agree this looks like the right answer. Rereading that code, what prepGlobalJsxRewrites says is that JSX.Element (with the global JSX namespace, not imported from anywhere) should get rewritten to that. So it seems likely that React.JSX.Element is intended to mean the same thing as JSX.Element.

So that function is a good place to look for a hint about what this type from React probably means. But I think the fix in principle is just another item in the list of rewrites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, OK. I've opened up the TsFlower code and I plan to make a PR doing that.

@chrisbobbe chrisbobbe force-pushed the pr-yarn-upgrade-2024-01-23 branch from c0c9054 to 7f437f3 Compare January 26, 2024 19:49
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushed, incorporating the improvement in the TsFlower tool; thanks for your help with that.

@chrisbobbe
Copy link
Contributor Author

Ah and I see I still need to run tools/verify-webview-js; I'll do that.

…write

See gnprice/tsflower#2 .

This rewrite will get used in a `yarn upgrade`, coming soon.
Release notes here:
  https://developer.android.com/build/releases/past-releases/agp-7-2-0-release-notes

Done because the upcoming `yarn upgrade` would otherwise cause a
build failure with a message that looks similar to one that
reportedly was resolved by upgrading to AGP 7.2.0:
  https://stackoverflow.com/a/74808149
and it turns out that our error is also resolved by upgrading. In
our case we upgraded to the latest 7.2.x, which right now is 7.2.2.

Our error message had this:

  gradle Error: Can't determine type for tag
  '<macro name="m3_comp_assist_chip_container_shape">?attr/shapeAppearanceCornerSmall</macro>'
Tested on Android (office device) and iOS (my iPhone). See the
previous commit about some preparation that was needed; we upgraded
the AGP.
@chrisbobbe chrisbobbe force-pushed the pr-yarn-upgrade-2024-01-23 branch from 7f437f3 to 1222bc9 Compare January 27, 2024 01:56
@chrisbobbe
Copy link
Contributor Author

OK, done; revision pushed.

@gnprice
Copy link
Member

gnprice commented Feb 16, 2024

Thanks! Looks good; merging.

@gnprice gnprice merged commit 1222bc9 into zulip:main Feb 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants