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

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

  • 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

@matticbot
Copy link
Contributor

matticbot commented Oct 3, 2019

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.

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 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 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
@@ -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 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
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants