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

Upgrade to use Node.js 20 and bump npm dependencies #2517

Merged
merged 16 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
60 changes: 60 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const jsdocConfig = require( '@wordpress/eslint-plugin/configs/jsdoc' );
const webpackConfig = require( './webpack.config' );

const webpackResolver = {
Expand Down Expand Up @@ -39,19 +40,78 @@ module.exports = {
getComputedStyle: 'readonly',
},
rules: {
'@wordpress/i18n-text-domain': [
'error',
{ allowedTextDomain: 'google-listings-and-ads' },
],
'@wordpress/no-unsafe-wp-apis': 1,
'react/react-in-jsx-scope': 'off',
'react-hooks/exhaustive-deps': [
'warn',
{
additionalHooks: 'useSelect',
},
],
// compatibility-code "WC < 7.6"
//
// Turn it off because:
// - `import { CurrencyFactory } from '@woocommerce/currency';`
// It's supported only since WC 7.6.0
// - `import { userEvent } from '@testing-library/user-event';`
// It works but the official documentation also recommends using the default export
'import/no-named-as-default': 'off',
'jest/expect-expect': [
'warn',
{ assertFunctionNames: [ 'expect', 'expect[A-Z]\\w*' ] },
],
// Turn it off temporarily because it involves a lot of re-alignment. We can revisit it later.
'jsdoc/check-line-alignment': 'off',
// The JS package `tracking-jsdoc` changes the definition of the `@fires` tag.
Copy link
Contributor

@puntope puntope Aug 22, 2024

Choose a reason for hiding this comment

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

Can you elaborate what do you mean by this?

The JS package tracking-jsdoc changes the definition of the @fires tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the code comment in c76514b. Let me know if it's still not clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will just write "Listing our event names to avoid eslint false alarms"

But not a blocker tho.

// List shared `@event` names to avoid false alarms.
'jsdoc/no-undefined-types': [
'error',
{
definedTypes: [
...jsdocConfig.rules[ 'jsdoc/no-undefined-types' ][ 1 ]
.definedTypes,
'gla_datepicker_update',
'gla_documentation_link_click',
'gla_faq',
'gla_filter',
'gla_google_account_connect_button_click',
'gla_google_mc_link_click',
'gla_launch_paid_campaign_button_click',
'gla_mc_account_switch_account_button_click',
'gla_modal_closed',
'gla_modal_open',
'gla_paid_campaign_step',
'gla_setup_ads',
'gla_setup_mc',
'gla_table_go_to_page',
'gla_table_page_click',
],
},
],
},
overrides: [
{
files: [ 'js/src/external-components/woocommerce/**' ],
rules: {
'@wordpress/i18n-text-domain': [
'error',
{ allowedTextDomain: 'woocommerce' },
],
},
},
{
files: [ 'js/src/external-components/wordpress/**' ],
rules: {
'@wordpress/i18n-text-domain': [
'error',
{ allowedTextDomain: '' },
],
},
},
{
files: [ 'tests/e2e/**/*.js' ],
rules: {
Expand Down
5 changes: 5 additions & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -1 +1,6 @@
engine-strict=true

# The legacy-peer-deps option eliminates React versioning conflicts in npm peer
# dependencies between @wordpress/* and @woocommerce/*. It should be removed
# after they don't have versioning conflicts.
legacy-peer-deps=true
2 changes: 1 addition & 1 deletion .nvmrc
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v16
v20
2 changes: 0 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ Now you can build the files using one of these commands:

Notice this repository has `engine-strict=true` directive set. That means you cannot install dependencies with other Node engines rather than the ones defined in the `engines` directive inside [package.json](./package.json). It's recommended to use [NVM](https://github.com/nvm-sh/nvm) and run `nvm use` before installing the dependencies to be sure you're using the recommended Node version.

We added Node `^18` and npm `^9` to allow dependabot to update our dependencies. But these are not supported versions.

## Working with DEWP

The Dependency Extraction Webpack Plugin makes working with frontend dependencies not so obvious, check [`Working with DEWP.md`](Working with DEWP.md) for more details.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,16 @@ const AttributeMappingDeleteRuleModal = ( { onRequestClose = noop, rule } ) => {
};

const handleClose = () => {
if ( deleting ) return;
if ( deleting ) {
return;
}
onRequestClose( 'dismiss' );
};

return (
<AppModal
onRequestClose={ handleClose }
title={ __( 'Delete attribute rule?', ' google-listings-and-ads' ) }
title={ __( 'Delete attribute rule?', 'google-listings-and-ads' ) }
buttons={ [
<AppButton
disabled={ deleting }
Expand Down
8 changes: 5 additions & 3 deletions js/src/attribute-mapping/attribute-mapping-rule-modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ const AttributeMappingRuleModal = ( { rule, onRequestClose = noop } ) => {
};

const handleClose = () => {
if ( saving ) return;
if ( saving ) {
return;
}
onRequestClose( 'dismiss' );
};

Expand All @@ -172,8 +174,8 @@ const AttributeMappingRuleModal = ( { rule, onRequestClose = noop } ) => {
className="gla-attribute-mapping__rule-modal"
title={
rule
? __( 'Manage attribute rule', ' google-listings-and-ads' )
: __( 'Create attribute rule', ' google-listings-and-ads' )
? __( 'Manage attribute rule', 'google-listings-and-ads' )
: __( 'Create attribute rule', 'google-listings-and-ads' )
}
buttons={ [
<AppButton
Expand Down
8 changes: 4 additions & 4 deletions js/src/components/app-button/index.scss
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
.app-button {
&[hidden] {
display: none !important;
}

white-space: nowrap;

svg {
Expand All @@ -14,6 +10,10 @@
}
}

&[hidden] {
display: none !important;
}

// Add a gap between the SVG icon and text when the icon is on the right side.
// `:last-child` is to prevent this style apply on the loading <Spinner>.
&--icon-position-right.has-icon svg:last-child {
Expand Down
14 changes: 7 additions & 7 deletions js/src/components/app-modal/index.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
.app-modal {
/**
* Set `hidden` as the default overflow for container and `auto` for body to
* make them stable across newer versions of @wordpress/components.
* More details can be found in the AppModal implementation.
*/
overflow: hidden;

@include break-large {
width: $gla-width-large;
}
Expand All @@ -19,13 +26,6 @@
}
}

/**
* Set `hidden` as the default overflow for container and `auto` for body to
* make them stable across newer versions of @wordpress/components.
* More details can be found in the AppModal implementation.
*/
overflow: hidden;

.components-modal__content {
overflow: auto;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
}

&__placeholder {
@include placeholder;

display: inline-block;
width: 18em;

@include placeholder;
Comment on lines -16 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary? I wonder if placeholder includes display or width props. Then it will be overrided

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this change necessary?

The change fixes a Sass deprecation warning. The explanation can be found in https://sass-lang.com/documentation/breaking-changes/mixed-decls/

image

I wonder if placeholder includes display or width props. Then it will be overrided

They don't have. Please refer to:

@mixin placeholder($lighten-percentage: 30%) {
animation: loading-fade 1.6s ease-in-out infinite;
background-color: $gray-100;
color: transparent;
&::after {
content: "\00a0";
}
@media screen and (prefers-reduced-motion: reduce) {
animation: none;
}
}

}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { renderHook } from '@testing-library/react-hooks';
import { renderHook } from '@testing-library/react';

/**
* Internal dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { useRef } from '@wordpress/element';
import { Form } from '@woocommerce/components';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it's worth it to import the component just for the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main difference is the code editor's reference hints will be better, apart from that there shouldn't be any difference. So it should be worth it.

Without import

image

With import
image

import { pick, noop } from 'lodash';

/**
Expand Down
12 changes: 6 additions & 6 deletions js/src/components/paid-ads/asset-group/images-selector.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe( 'ImagesSelector', () => {
render( <ImagesSelector imageConfig={ imageConfig } /> );
await userEvent.click( getAddButton() );

expect( openSelector ).toBeCalledWith( undefined );
expect( openSelector ).toHaveBeenCalledWith( undefined );
} );

it( 'When clicking on a replace image button, the `openSelector` should be called with the corresponding image ID.', async () => {
Expand All @@ -88,7 +88,7 @@ describe( 'ImagesSelector', () => {
const buttons = getReplaceButtons();
await userEvent.click( buttons.at( 0 ) );

expect( openSelector ).toBeCalledWith( urlA );
expect( openSelector ).toHaveBeenCalledWith( urlA );
} );

it( 'When clicking on a remove image button, the corresponding image should be removed.', async () => {
Expand All @@ -103,7 +103,7 @@ describe( 'ImagesSelector', () => {
await userEvent.click( buttons.at( 1 ) );

expect( getImgUrls() ).toEqual( [ urlA, urlC ] );
expect( onChange ).toBeCalledWith( [ urlA, urlC ] );
expect( onChange ).toHaveBeenCalledWith( [ urlA, urlC ] );
} );

it( 'When the number of images reaches the maximum limit, it should disable the add image button and vice versa.', async () => {
Expand Down Expand Up @@ -195,7 +195,7 @@ describe( 'ImagesSelector', () => {
await act( async () => onSelect( imageB ) );

expect( getImgUrls() ).toEqual( [ urlA, urlB ] );
expect( onChange ).toBeCalledWith( [ urlA, urlB ] );
expect( onChange ).toHaveBeenCalledWith( [ urlA, urlB ] );
} );

it( 'When an image is called back for deletion, the image with the same ID should be removed from the image list.', async () => {
Expand All @@ -209,7 +209,7 @@ describe( 'ImagesSelector', () => {
await act( async () => onDelete( imageB ) );

expect( getImgUrls() ).toEqual( [ urlA, urlC ] );
expect( onChange ).toBeCalledWith( [ urlA, urlC ] );
expect( onChange ).toHaveBeenCalledWith( [ urlA, urlC ] );
} );

it( 'When an image called back for addition already has one with the same ID in the image list, it should not be added.', async () => {
Expand Down Expand Up @@ -238,7 +238,7 @@ describe( 'ImagesSelector', () => {
await act( async () => onSelect( imageC ) );

expect( getImgUrls() ).toEqual( [ urlC, urlB ] );
expect( onChange ).toBeCalledWith( [ urlC, urlB ] );
expect( onChange ).toHaveBeenCalledWith( [ urlC, urlB ] );
} );

it( 'When an image is called back for replacement but the previously clicked image has been removed from the image list, it should be pushed to the image list.', async () => {
Expand Down
4 changes: 3 additions & 1 deletion js/src/components/tree-select-control/checkbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ import { Icon, check } from '@wordpress/icons';
* @return {JSX.Element|null} The Checkbox component
*/
const Checkbox = ( { option, checked, className, ...props } ) => {
if ( ! option ) return null;
if ( ! option ) {
return null;
}

return (
<div className={ className }>
Expand Down
4 changes: 3 additions & 1 deletion js/src/components/tree-select-control/control.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ const Control = forwardRef(

const handleKeydown = ( event ) => {
if ( BACKSPACE === event.key ) {
if ( value ) return;
if ( value ) {
return;
}
onTagsChange( tags.slice( 0, -1 ) );
event.preventDefault();
}
Expand Down
54 changes: 30 additions & 24 deletions js/src/components/tree-select-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { ARROW_DOWN, ARROW_UP, ENTER, ESCAPE, ROOT_VALUE } from './constants';
],
}
],
**/
*/

/**
* @typedef {Object} CommonOption
Expand Down Expand Up @@ -190,7 +190,9 @@ const TreeSelectControl = ( {
const highlightOptionLabel = ( optionLabel, matchPosition ) => {
const matchLength = matchPosition + filter.length;

if ( ! isSearching ) return optionLabel;
if ( ! isSearching ) {
return optionLabel;
}

return (
<span>
Expand Down Expand Up @@ -309,7 +311,9 @@ const TreeSelectControl = ( {
}, [ treeOptions, filter ] );

const onKeyDown = ( event ) => {
if ( disabled ) return;
if ( disabled ) {
return;
}

if ( ESCAPE === event.key ) {
setTreeVisible( false );
Expand Down Expand Up @@ -376,26 +380,6 @@ const TreeSelectControl = ( {
);
};

/**
* Handles a change on the Tree options. Could be a click on a parent option
* or a child option
*
* @param {boolean} checked Indicates if the item should be checked
* @param {InnerOption} option The option to change
*/
const handleOptionsChange = ( checked, option ) => {
if ( option.hasChildren ) {
handleParentChange( checked, option );
} else {
handleSingleChange( checked, option );
}

setInputControlValue( '' );
if ( ! nodesExpanded.includes( option.parent ) ) {
controlRef.current.focus();
}
};

/**
* Handles a change of a child element.
*
Expand Down Expand Up @@ -434,6 +418,26 @@ const TreeSelectControl = ( {
onChange( newValue );
};

/**
* Handles a change on the Tree options. Could be a click on a parent option
* or a child option
*
* @param {boolean} checked Indicates if the item should be checked
* @param {InnerOption} option The option to change
*/
const handleOptionsChange = ( checked, option ) => {
if ( option.hasChildren ) {
handleParentChange( checked, option );
} else {
handleSingleChange( checked, option );
}

setInputControlValue( '' );
if ( ! nodesExpanded.includes( option.parent ) ) {
controlRef.current.focus();
}
};

/**
* Handles a change of a Tag element. We map them to Value format.
*
Expand Down Expand Up @@ -481,7 +485,9 @@ const TreeSelectControl = ( {
setTreeVisible( true );
} }
onControlClick={ () => {
if ( disabled ) return;
if ( disabled ) {
return;
}
setTreeVisible( true );
} }
instanceId={ instanceId }
Expand Down
Loading
Loading