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

chore: update memize to v2 #50172

Merged
merged 3 commits into from
May 15, 2023
Merged

Conversation

johnhooks
Copy link
Contributor

What?

Update memize to the latest version.

Why?

Typing @wordpress/blocks and @wordpress/block-editor depend on a change to how memize exports it's types. There are also improvements to the generic function argument of memize that will facilitate more complete typing.

Testing Instructions

There are two failing tests in packages/edit-site/src/store/test/utils.js which rely on an internal testing API of memize, getCache(). It is no longer included in the published version. Either the tests need to be removed or tested another way (temporally they are commented out).

Notes

An update to how v2 of memize defines its exports being worked on in aduth/memize#11, it will need to be published before the unit tests will pass.

@johnhooks
Copy link
Contributor Author

johnhooks commented Apr 28, 2023

@gziolo what is the right way to update a dependency version within the setup of this monorepo. I'm getting very different results between my local computer and the CI. I think it has to do with the lock file.

What I did

  • Update the version of memize in the package.json files of all the packages that declare it as a dependency.
  • Run npm install
  • For some reason memize isn't being installed and is actually being removed from the package-lock.json

Edit:

Sorry I made some mistake, it's all correct now I believe.

Edit 2:

No there is still some issue, what is the correct version of npm to use? I'm currently using what is installed with Node v14.21.3 which is npm v6.14.18

@gziolo
Copy link
Member

gziolo commented Apr 28, 2023

memize needs to be updated in the top-level node_modules. The simplest way is to run npm install memize@2.0.1 in the project so the lock file gets correctly updated. Once that happens, it needs to be removed from the package.json and another npm install run will clean up everything.

@gziolo
Copy link
Member

gziolo commented May 5, 2023

The lock file is still incorrect. It doesn’t contain the latest version of memize in all places.

I tried to apply a patch from this branch locally, but I ran into the issue when using the latest trunk:

Screenshot 2023-05-05 at 09 38 25

@gziolo gziolo added npm Packages Related to npm packages [Type] Code Quality Issues or PRs that relate to code quality labels May 5, 2023
@johnhooks
Copy link
Contributor Author

@gziolo I will try reverting the lockfile to the original and using your trick

@dcalhoun
Copy link
Member

dcalhoun commented May 5, 2023

If capturing correct lock file changes while upgrading memize continues to be an issue, removing and re-adding the dependency may also help.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @johnhooks for bringing it to the finish line!

@gziolo gziolo merged commit 71c18bd into WordPress:trunk May 15, 2023
48 of 49 checks passed
@github-actions github-actions bot added this to the Gutenberg 15.9 milestone May 15, 2023
@johnhooks
Copy link
Contributor Author

johnhooks commented May 15, 2023

@gziolo there were two commented out tests that used an internal api of memize that was no longer exported, I hadn't received feedback on what to do with them. I think they could be safely removed because they are effectively testing the functionality of memize. Should I do another PR that completely removes the two tests, or should we write the tests differently?

https://github.com/johnhooks/gutenberg/blob/6c452bcaa43db5938d8902d44bd4bdf2081dc5d2/packages/edit-site/src/store/test/utils.js#L147-L155

https://github.com/johnhooks/gutenberg/blob/6c452bcaa43db5938d8902d44bd4bdf2081dc5d2/packages/edit-site/src/store/test/utils.js#L181-L188

@gziolo
Copy link
Member

gziolo commented May 16, 2023

Should I do another PR that completely removes the two tests, or should we write the tests differently?

I missed that, along with other changes. Another PR with the cleanup would be more than welcomed 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants