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

[ETK] Update @wordpress/interface to the latest published version #73629

Closed
wants to merge 15 commits into from
Closed

[ETK] Update @wordpress/interface to the latest published version #73629

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c11b40c
Update ``@wordpress/interface` to the latest published version
fullofcaffeine Feb 22, 2023
3f26ef9
Dedupe NPM packages
fullofcaffeine Feb 22, 2023
6335888
Silence peerDependency not satisfied warning for @wordpress/interface
fullofcaffeine Feb 22, 2023
4e130bd
Override `@wordpress/element` subpackages to 4.2.0 for storybook to c…
fullofcaffeine Feb 22, 2023
8f5d8f8
Fix test that failed due to `Date.now` being `undefined`
fullofcaffeine Feb 22, 2023
a9d298c
Cleanup .yarnrc.yml
fullofcaffeine Feb 22, 2023
7317b4a
Dedupe
fullofcaffeine Feb 22, 2023
b090bcf
Reinstall/update `@wordpress/interface` without deduping
fullofcaffeine Feb 23, 2023
55213d0
Revert yarn.lock
fullofcaffeine Feb 23, 2023
8a0490f
Merge branch 'trunk' into update/etk-wordpress-interface-to-latest-ve…
fullofcaffeine Feb 23, 2023
3ec8d39
Reinstall packages after rebase
fullofcaffeine Feb 23, 2023
42fd038
Try with more strict @wordpress dependencies
tyxla Feb 24, 2023
44249e1
Bump some obsolete package versions
tyxla Feb 24, 2023
a11fc56
Revert "Bump some obsolete package versions"
tyxla Feb 24, 2023
3a6e031
Revert "Try with more strict @wordpress dependencies"
tyxla Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .yarnrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ logFilters:
pattern: "@automattic/global-styles@workspace:packages/global-styles provides react-dom (*) with version 17.0.2, which doesn't satisfy what @wordpress/block-editor and some of its descendants request"
- level: discard
pattern: "@automattic/global-styles@workspace:packages/global-styles provides react-dom (*) with version 17.0.2, which doesn't satisfy what @wordpress/components and some of its descendants request"
- level: discard
pattern: "@automattic/wpcom-editing-toolkit@workspace:apps/editing-toolkit provides react (p3131a) with version 17.0.2, which doesn't satisfy what @wordpress/interface and some of its descendants request"
- level: discard
pattern: "@automattic/wpcom-editing-toolkit@workspace:apps/editing-toolkit provides react-dom (pd5894) with version 17.0.2, which doesn't satisfy what @wordpress/interface and some of its descendants request"

nodeLinker: node-modules

Expand Down
2 changes: 1 addition & 1 deletion apps/editing-toolkit/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
"@wordpress/html-entities": "^3.9.0",
"@wordpress/i18n": "^4.9.0",
"@wordpress/icons": "^9.0.0",
"@wordpress/interface": "^4.8.0",
"@wordpress/interface": "^5.4.0",
"@wordpress/is-shallow-equal": "^4.9.0",
"@wordpress/keycodes": "^3.9.0",
"@wordpress/notices": "^3.10.0",
Expand Down
12 changes: 10 additions & 2 deletions client/blocks/time-mismatch-warning/test/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,21 @@ jest.mock( 'calypso/state/preferences/selectors', () => ( {
getPreference: jest.fn( () => false ),
} ) );

class MockedDate extends Date {
getTimezoneOffset() {
return 240;
}
}

const originalDate = global.Date;

describe( 'TimeMismatchWarning', () => {
beforeAll( () => {
jest.spyOn( global, 'Date' ).mockImplementation( () => ( { getTimezoneOffset: () => 240 } ) );
global.Date = MockedDate;
Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These tests were failing with:

   TypeError: Date.now is not a function                                                                                                                                                                           
                                                                                                                                                                                                                    
      at ../node_modules/requestidlecallback/index.js:57:21                                                                                                                                                         
      at MutationObserver.onInputorMutation (../node_modules/requestidlecallback/index.js:87:3)

I haven't dug too deep, but looks like this is caused by the requestidlecallback package, which now uses Date.now . Before the package updates that are part of this PR, Date.now was not in any of the code paths for this test (apparently), so something changed after the update.

With the mockedImplementation approach, it discarded Date's original implementation to only define the custom getTimezoneOffset. I've tried to use spyOn + mockImplementation to return a custom class based off Date but failed - it was much simpler to just override the global variable and re-assign the original object to it in the afterAll callback.

I wonder why this test triggered a portion of code that uses this package now while it didn't before the update. Maybe that package was already used, and it was recently updated to use Date.now. Provided tests pass, I think it's okay to consider this relatively safe to ship, but if you have more insights, let me know. Besides, in the case of globals like Date, I think it's safer to keep the original implementation accessible and mock the functions over it. The previous approach based on mockImplementation removed all other functions and properties (including the static now).

Copy link
Member

Choose a reason for hiding this comment

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

No concerns from my side about this change. We've seen similar issues in Gutenberg tests too. It's usually that something tries to access Date before the jsdom mock has been setup, or something inadvertently overrides it for the test.

} );

afterAll( () => {
jest.spyOn( global, 'Date' ).mockRestore();
global.Date = originalDate;
} );

test( 'to render nothing if no site ID is provided', () => {
Expand Down
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,9 @@
"newspack-components/@wordpress/element": "4.7.0",
"newspack-components/@wordpress/i18n": "4.9.0",
"keytar@npm:7.7.0/node-addon-api": "3.1.0",
"lzma-native": "^8.0.5"
"lzma-native": "^8.0.5",
"@wordpress/primitives/@wordpress/element": "4.20.0",
Copy link
Member

Choose a reason for hiding this comment

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

Should we pin this to a particular version of @wordpress/primitives to avoid this propagating to another version of @wordpress/primitives and accidentally into the rest of the monorepo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍🏻

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, after resetting the yarn.lock to the trunk version and re-installing the new version of @wordpress/interface in ETK without deduping, these resolution overrides don't seem to be needed anymore, the storybook task is working fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed those entries.

"@wordpress/icons@npm:9.18.0/@wordpress/element": "4.20.0"
},
"packageManager": "yarn@3.2.3",
"dependenciesMeta": {
Expand Down
Loading