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.

Sign up for GitHub

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

Jump to bottom

TSC: Fix unsupported TS syntax in getTransformedStreamItems selector #73991

Merged
merged 1 commit into from
Mar 2, 2023
Merged

TSC: Fix unsupported TS syntax in getTransformedStreamItems selector #73991

merged 1 commit into from
Mar 2, 2023

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Mar 2, 2023 •
edited
Loading

Proposed Changes

While working on #73890 I stumbled upon an error that I see when I run yarn tsc --noEmit --project client/tsconfig.json:

Screenshot 2023-03-02 at 14 02 17

The fix is straightforward: we're renaming the offending file from .js to .ts and specifying a type for the deps argument of the treeSelect's selector argument. Without the type, TS was crashing.

This has also been reported in #66894.

Fixes #66894.

Testing Instructions

  • Checkout latest trunk locally.
  • Run yarn tsc --noEmit --project client/tsconfig.json
  • Verify you get the error from above.
  • Now checkout this branch.
  • Run the command again.
  • Verify that it passes without errors (nothing should be output by the command).
  • Smoke test the reader and a few different stream types.
  • Verify that all checks are green. There are e2e tests that test the reader that would fail if we broke this selector.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@tyxla tyxla added [Type] Bug Framework [Type] Tooling Related to tools, scripts, automation, DevX, etc. labels Mar 2, 2023
@tyxla tyxla self-assigned this Mar 2, 2023
@tyxla tyxla requested a review from a team as a code owner March 2, 2023 12:06
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 2, 2023
@tyxla tyxla requested review from sirbrillig, a team and danielbachhuber March 2, 2023 12:06
@github-actions
Copy link

github-actions bot commented Mar 2, 2023 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-58972
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-58972&env=jetpack

@tyxla tyxla mentioned this pull request Mar 2, 2023
Closed
@matticbot
Copy link
Contributor

matticbot commented Mar 2, 2023 •
edited
Loading

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@tyxla tyxla marked this pull request as draft March 2, 2023 12:22
@tyxla
Copy link
Member Author

tyxla commented Mar 2, 2023

Made this a draft as I'm still working on it.

@tyxla tyxla force-pushed the fix/mysterious-ts-debug-failure branch from fb78ad4 to 758f84f Compare March 2, 2023 12:35
tyxla
tyxla commented Mar 2, 2023
View reviewed changes
Copy link
Member Author

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is ready for a review 👀

Doing a self-review, too.

packages/tree-select/src/index.ts
@@ -26,7 +26,7 @@ interface Options< A extends unknown[] > {
getCacheKey?: GenerateCacheKey< A >;
}

interface CachedSelector< S, A extends unknown[], R = unknown > {
export interface CachedSelector< S, A extends unknown[], R = unknown > {
Copy link
Member Author

Choose a reason for hiding this comment

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

Necessary as without it we were also getting this error:

Exported variable 'getTransformedStreamItems' has or is using name 'CachedSelector' from external module "/calypso/packages/tree-select/src/index" but cannot be named.

client/state/reader/streams/selectors/get-reader-stream-transformed-items.ts
getReaderStream( state, streamKey ).items,
getReaderStream( state, recsStreamKey ).items,
getReaderFollows( state ),
],
( [ items, recs, follows ] ) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
( [ items, recs, follows ]: Array< any > ) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the magic. For some reason, TS needs this type explicitly, although it's properly specified in treeSelect's types.

client/state/reader/streams/selectors/get-reader-stream-transformed-items.ts
@@ -17,12 +17,13 @@ import 'calypso/state/reader/init';
* function( state, { streamKey: string, recsStreamKey: string }): Array
*/
export const getTransformedStreamItems = treeSelect(
( state, { streamKey, recsStreamKey } ) => [
( state, { streamKey, recsStreamKey }: { streamKey: string; recsStreamKey: string } ) => [
Copy link
Member Author

Choose a reason for hiding this comment

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

Necessary only to complete the type definition. Went the simple inline way, no need for abstractions IMHO.

@tyxla tyxla marked this pull request as ready for review March 2, 2023 12:48
danielbachhuber
danielbachhuber approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

This change resolves the unexpected crash for me. The code changes seem fine, although I don't feel 100% qualified to assess them.

image

image

sirbrillig
sirbrillig approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

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

This seems to work and I can run tsc again!

Initsogar
Initsogar approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@Initsogar Initsogar left a comment

Choose a reason for hiding this comment

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

Tested locally, and now I can runyarn run typecheck (yarn run tsc --project client).
Thanks for jumping in and helping us to fix it. :)

@tyxla tyxla merged commit 1351bd5 into trunk Mar 2, 2023
@tyxla tyxla deleted the fix/mysterious-ts-debug-failure branch March 2, 2023 18:05
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirbrillig sirbrillig sirbrillig approved these changes

@Initsogar Initsogar Initsogar approved these changes

@danielbachhuber danielbachhuber danielbachhuber approved these changes

Assignees

@tyxla tyxla

Labels
Framework [Type] Bug [Type] Tooling Related to tools, scripts, automation, DevX, etc.
Projects
None yet
Milestone
No milestone
5 participants
@tyxla @matticbot @danielbachhuber @Initsogar @sirbrillig