Skip to content

chore(dev-deps): update @automattic/eslint-plugin-wpvip #62

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

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

sjinks
Copy link
Member

@sjinks sjinks commented Dec 9, 2023

Update @automattic/eslint-plugin-wpvip to 0.9.1, remove extraneous eslint plugins, fix the code.

I used Automattic/vip-cli#1577 as a template.

@sjinks sjinks self-assigned this Dec 9, 2023
@sjinks sjinks requested a review from gudmdharalds December 9, 2023 22:52
@github-actions github-actions bot added the semver-patch nlm assigned semver patch update label Dec 9, 2023
async function testHarness( replacements, customScript = null ) {
debug( replacements, customScript );
const binary = process.env.CI === 'true' ? './bin/go-search-replace-test' : null;
const script = customScript || binary;
const result = await replace( readableStream, replacements, script );
await new Promise( resolve => {

await /** @type {Promise<void>} */ ( new Promise( ( resolve ) => {
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 weird syntax is to keep the TS validator happy.

Ref: microsoft/TypeScript#46570 (comment)

Comment on lines -113 to +117
it( 'throws a new Error when the script/binary returns a non-zero exit code', async () => {
try {
// await replace( readableStream, [ 'thisdomain.com', 'thatdomain.com' ], nonZeroExitCodeScript );
await testHarness( [ 'thisdomain.com', 'thatdomain.com' ], nonZeroExitCodeScript );
} catch ( err ) {
expect( err ).toEqual( 'The search and replace process exited with a non-zero exit code: 1' );
}
} );
it( 'throws a new Error when the script/binary returns a non-zero exit code', () =>
expect( testHarness( [ 'thisdomain.com', 'thatdomain.com' ], nonZeroExitCodeScript ) ).rejects.toEqual( 'The search and replace process exited with a non-zero exit code: 1' ),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Our ESLint configuration does not like conditional expect. Luckily, we can replace the try...catch block with the .rejects matcher.

Comment on lines -212 to +209
fsMkdirSpy.mockResolvedValue();
fsMkdirSpy.mockResolvedValue( undefined );
Copy link
Member Author

@sjinks sjinks Dec 9, 2023

Choose a reason for hiding this comment

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

Here and below: the function is expected to return something (in this case, undefined is fine). While the lack of value is technically undefined, the linter does not like this.

Comment on lines -49 to +52
function getLatestReleaseUrlForPlatformAndArch( { platform, arch } = {} ) {
function getLatestReleaseUrlForPlatformAndArch( { platform = '', arch = '' } = {} ) {
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 linter insisted on having a default value.

Comment on lines +67 to 70
/**
* @param {{ arch?: string, platform?: string }} options
*/
async function downloadBinary( { arch = process.arch, platform = process.platform } = {} ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TIL that this works. Although there is no parameter named options. Oh well.

Comment on lines -123 to +139
return new Promise( async ( resolve, reject ) => {
let responseStream;
try {
responseStream = await downloadBinary( { platform, arch } );
} catch ( downloadErr ) {
debug( { downloadErr } );
return reject( downloadErr );
}
let responseStream;
try {
responseStream = await downloadBinary( { platform, arch } );
} catch ( downloadErr ) {
debug( { downloadErr } );
return Promise.reject( downloadErr );
}
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 because of the no-async-promise-executor rule. The suggestion was to reduce the scope of the Promise constructor, which worked.

@@ -21,17 +21,10 @@
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe keep external/internal dependencies comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

We removed them all in VIP CLI, I tried to be consistent :-)

@@ -1,17 +1,12 @@
/**
* External dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe keep external/internal dependencies comment?

@sjinks sjinks merged commit 78a4bfc into master Dec 11, 2023
@sjinks sjinks deleted the update/eslint-config-wpvip branch December 11, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch nlm assigned semver patch update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants