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

Type: tree-select package #36493

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Type: tree-select package #36493

merged 2 commits into from
Apr 7, 2021

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Oct 3, 2019 •
edited
Loading

  • Translate tree-select to TypeScript.
  • Will generate typings and allow them to be published/used in Calypso.

props to @beaucollins who was the original author of most of the typings.

Testing instructions

Confirm that clean produces a clean directory (nothing in packages/tree-select/{dist,types}).
Confirm that build produces expected output (CommonJS in packages/tree-select/dist/cjs and ECMAScript Module in corresponding …/esm directory.}

Compare with most recent published builds:

The results should be similar but not identical.

# Clean
npx lerna run clean --scope="*/tree-select"
# Build
npx lerna run prepare --scope="*/tree-select"

@sirreal sirreal self-assigned this Oct 3, 2019
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/type-tree-select

@matticbot
Copy link
Contributor

matticbot commented Oct 3, 2019 •
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.

@sirreal sirreal force-pushed the update/type-tree-select branch from 41c8a25 to b8b29ca Compare October 7, 2019 16:11
sirreal
sirreal commented Oct 17, 2019
View reviewed changes
packages/tree-select/tsconfig.json Show resolved Hide resolved
packages/tree-select/tsconfig.json Outdated Show resolved Hide resolved
packages/tree-select/tsconfig.json Outdated Show resolved Hide resolved
packages/tree-select/tsconfig-cjs.json Outdated Show resolved Hide resolved
packages/tree-select/tsconfig.json Outdated Show resolved Hide resolved
packages/tree-select/src/index.ts Outdated Show resolved Hide resolved
@sirreal sirreal mentioned this pull request Nov 7, 2019
Open
@sirreal sirreal force-pushed the update/type-tree-select branch 3 times, most recently from 013eb10 to 0cb0e15 Compare November 23, 2019 21:04
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build Packages and removed [Status] In Progress labels Nov 23, 2019
@sirreal sirreal marked this pull request as ready for review November 23, 2019 21:12
@sirreal sirreal requested a review from a team as a code owner November 23, 2019 21:12
@sirreal sirreal requested a review from a team November 23, 2019 21:12
@sirreal sirreal added [Status] Blocked / Hold [Status] Needs Rebase and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 27, 2020
@sirreal
Copy link
Member Author

sirreal commented Feb 27, 2020

Putting this on hold. The TS translation here is handy, but I'd like to apply my learnings when I get WordPress/gutenberg#18942 sorted.

@sarayourfriend sarayourfriend changed the base branch from master to trunk November 20, 2020 16:14
@sirreal sirreal force-pushed the update/type-tree-select branch from 0cb0e15 to 82b0b7d Compare December 29, 2020 11:09
@sirreal sirreal requested a review from a team December 29, 2020 11:12
@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 Dec 29, 2020
@sirreal sirreal requested a review from a team December 29, 2020 11:12
sirreal
sirreal commented Dec 29, 2020
View reviewed changes
packages/tree-select/src/index.ts
@@ -76,7 +110,8 @@ const NULLISH_KEY = {};
* Note: Inserts WeakMaps except for the last map which will be a regular Map.
* The last map is a regular one because the the key for the last map is the string results of args.join().
*/
function insertDependentKey( map, key, currentIndex, arr ) {
function insertDependentKey( map: any, key: unknown, currentIndex: number, arr: 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.

This is an unusual reducer function, it consumes an array of keys to walk a tree data structure (WeakMaps of WeakMaps) until it reaches a leaf which is a Map< string, CachedSelectorResultValue >.

I've typed map as any because usage suggests this internal function is working well enough and it was difficult to type it as Map<…> | WeakMap<…> here.

@sirreal
Copy link
Member Author

sirreal commented Dec 29, 2020

I've redone this according to latest best practices in Calypso and it should be ready to move forward.

@sirreal sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 4, 2021
@sirreal sirreal force-pushed the update/type-tree-select branch from d7c09c3 to 7230688 Compare January 4, 2021 09:31
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 4, 2021
@sarayourfriend sarayourfriend force-pushed the update/type-tree-select branch from 7230688 to b7d80ab Compare April 2, 2021 12:55
@sirreal
Copy link
Member Author

sirreal commented Apr 7, 2021

Thanks for the changes and review @sarayourfriend! Since I'm a bit distanced from this work right now, I'd be most comfortable if you handle landing this at your leisure.

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 7, 2021
@sarayourfriend sarayourfriend merged commit 6030f61 into trunk Apr 7, 2021
@sarayourfriend sarayourfriend deleted the update/type-tree-select branch April 7, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beaucollins beaucollins beaucollins left review comments

@blowery blowery blowery left review comments

@sarayourfriend sarayourfriend sarayourfriend approved these changes

Assignees

@sirreal sirreal

Labels
Build Packages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sirreal @matticbot @blowery @beaucollins @sarayourfriend