Boost: Fix linting config and standards in files #21015

davidlonjon · 2021-09-09T13:47:29Z

Following quite a few experimentations with linting due to #20996 I realize that a few linting related configurations and standards had to be fixed with Boost.

Changes proposed in this Pull Request:

Remove an old eslint configuration
Add a new .eslintignore file to whitelist the global.d.ts file so it can be linted. Also adds it to the top of the Jetpack Monorepo .eslintignore. This in effect disabling the rule to ignore every d.ts files from ( I think) the @wordpress/eslint-plugin. I am whitelisting it because it seems we missed a few issues from that file as it was not linted. I am not sure if there is a reason not to lint it.
Fix the global.d.ts file by fixing an import path and removing an import that was not used anymore.
Fix a couple of paths in the tsconfig file
Fix a type in app/assets/src/js/utils/generate-critical-css.ts that somehow was missed.

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

N/A

Testing instructions:

Make sure the lining on the local environment still passes as well as the one on the CI.

…ting

github-actions · 2021-09-09T13:49:42Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Boost plugin:

Next scheduled release: October 5, 2021.
Scheduled code freeze: September 27, 2021.

anomiex · 2021-09-09T15:51:07Z

.eslintignore

@@ -40,6 +40,7 @@ projects/plugins/boost/app/assets/dist/**
 projects/plugins/boost/tests/**
 projects/plugins/boost/docker/**
 projects/plugins/boost/wordpress/**
+!projects/plugins/boost/app/assets/src/js/*.d.ts


I see there are two .d.ts files in the repo, projects/plugins/boost/app/assets/src/js/global.d.ts and projects/plugins/boost/app/assets/src/js/svg/svgs.d.ts. This rule only matches the first.

If you want to match both, you could do like

Suggested change

!projects/plugins/boost/app/assets/src/js/*.d.ts

!projects/plugins/boost/app/assets/src/js/**/*.d.ts

And similarly in your local .eslintignore.

Yep, tested and the more globby path also catches svgs.d.ts. Nice catch! Committed in 91db7f7

Thank you @anomiex for taking a look at the PR and spotting this one 🙏🏻
Also thank you @thingalon 👍

I have added the globby path to the local .eslintignore file too in commit 581e224

thingalon

👍 works well, looks good to me. Thanks @davidlonjon !

…intingore

thingalon

Still looking good!

anomiex

Seems ok to me, but needs a master merge.

You might consider whether you need local linting at all rather than making sure the monorepo's linting checks everything, since the local linting is not run in CI. But that's not something that needs to hold up this PR.

* master: (32 commits) jetpack: Work around another calypso-build oddity (#21004) Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922) Search: Improve filter label alignment for longer terms (#21008) Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993) Notifications: fix DailyMotion shortcode render in Calypso (#21032) Notifications: Fix VR shortcode render (#20871) E2E tests: fix for logging Jetpack version (#21054) Admin Menu: Fix skipped tests (#21055) Admin Menu: Sanitize current screen identifier (#21049) Fix/phpunit failed on multisite (#21053) Boost: Fix Critical CSS resume on module re-enable (#21031) Update wordpress monorepo (#21021) Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045) Publicize: move it to plugins folder (#21016) SEO Tools: add archive_title page title token (#20920) Unit tests: Fix silent errors in Jetpack tests (#21030) actions: Don't run Beta Plugin artifact action on forked PRs (#21019) Jetpack Sync: Fix broken Sync unit tests (#21037) Notifications: Fix SoundCloud shortcode render (#20869) Jetpack Assistant: Correct purchased product feature class (#21014) ...

davidlonjon · 2021-09-14T02:17:03Z

Seems ok to me, but needs a master merge.

You might consider whether you need local linting at all rather than making sure the monorepo's linting checks everything, since the local linting is not run in CI. But that's not something that needs to hold up this PR.

Thanks @anomiex. Yes, the next step would be to have all the lining done by the Monorepo's linting. I have just created now this #21066 for it. I will implement it after this other linting related PR fixing local linting is also merged.

# By Ivan Ottinger (4) and others # Via GitHub * master: (28 commits) Boost: Fix linting config and standards in files (#21015) jetpack: Work around another calypso-build oddity (#21004) Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922) Search: Improve filter label alignment for longer terms (#21008) Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993) Notifications: fix DailyMotion shortcode render in Calypso (#21032) Notifications: Fix VR shortcode render (#20871) E2E tests: fix for logging Jetpack version (#21054) Admin Menu: Fix skipped tests (#21055) Admin Menu: Sanitize current screen identifier (#21049) Fix/phpunit failed on multisite (#21053) Boost: Fix Critical CSS resume on module re-enable (#21031) Update wordpress monorepo (#21021) Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045) Publicize: move it to plugins folder (#21016) SEO Tools: add archive_title page title token (#20920) Unit tests: Fix silent errors in Jetpack tests (#21030) actions: Don't run Beta Plugin artifact action on forked PRs (#21019) Jetpack Sync: Fix broken Sync unit tests (#21037) Notifications: Fix SoundCloud shortcode render (#20869) ... # Conflicts: # projects/plugins/boost/tsconfig.json

…workflow # By Brad Jorsch (6) and others # Via GitHub * master: (53 commits) Boost: Fix linting config and standards in files (#21015) jetpack: Work around another calypso-build oddity (#21004) Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922) Search: Improve filter label alignment for longer terms (#21008) Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993) Notifications: fix DailyMotion shortcode render in Calypso (#21032) Notifications: Fix VR shortcode render (#20871) E2E tests: fix for logging Jetpack version (#21054) Admin Menu: Fix skipped tests (#21055) Admin Menu: Sanitize current screen identifier (#21049) Fix/phpunit failed on multisite (#21053) Boost: Fix Critical CSS resume on module re-enable (#21031) Update wordpress monorepo (#21021) Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045) Publicize: move it to plugins folder (#21016) SEO Tools: add archive_title page title token (#20920) Unit tests: Fix silent errors in Jetpack tests (#21030) actions: Don't run Beta Plugin artifact action on forked PRs (#21019) Jetpack Sync: Fix broken Sync unit tests (#21037) Notifications: Fix SoundCloud shortcode render (#20869) ... # Conflicts: # projects/plugins/boost/tsconfig.json # projects/plugins/jetpack/tests/e2e/bin/env.sh

* Remove an old eslint config file not in use anymore * Add a new eslintignore file to whitelist the global.d.ts file for linting * Whitelist the global.d.ts Boost plugin file for linting * Fix typeRoots and include paths * Fix issue with incorrect path and unused import * Fix type * Add changelog * Use a more globby path to catch all typescript def files * Use a more globby path to catch all typescript def files in Boost eslintingore Co-authored-by: Mark George <thingalon@gmail.com>

davidlonjon added 7 commits September 9, 2021 20:28

Remove an old eslint config file not in use anymore

4de3077

Add a new eslintignore file to whitelist the global.d.ts file for lin…

0887c14

…ting

Whitelist the global.d.ts Boost plugin file for linting

b663d02

Fix typeRoots and include paths

0adb700

Fix issue with incorrect path and unused import

dfd935c

Fix type

e959e33

Add changelog

6aaf0f9

davidlonjon added [Type] Janitorial [Plugin] Boost A feature to speed up the site and improve performance. labels Sep 9, 2021

davidlonjon requested review from jeherve and thingalon September 9, 2021 13:47

davidlonjon self-assigned this Sep 9, 2021

davidlonjon added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review Touches WP.com Files labels Sep 9, 2021

matticbot removed the Touches WP.com Files label Sep 9, 2021

anomiex reviewed Sep 9, 2021

View reviewed changes

Use a more globby path to catch all typescript def files

91db7f7

thingalon previously approved these changes Sep 10, 2021

View reviewed changes

thingalon removed the [Status] Needs Team Review label Sep 10, 2021

Use a more globby path to catch all typescript def files in Boost esl…

581e224

…intingore

davidlonjon dismissed thingalon’s stale review via 581e224 September 10, 2021 05:03

davidlonjon requested review from thingalon and anomiex September 10, 2021 05:07

davidlonjon mentioned this pull request Sep 10, 2021

Boost: Fix TypeScript linting setup and issues #21026

Merged

thingalon approved these changes Sep 13, 2021

View reviewed changes

anomiex approved these changes Sep 13, 2021

View reviewed changes

anomiex added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Sep 13, 2021

davidlonjon merged commit 7f0ca6a into master Sep 14, 2021

davidlonjon deleted the fix/boost-linting-setup-and-standards branch September 14, 2021 02:23

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Boost: Fix linting config and standards in files #21015

Boost: Fix linting config and standards in files #21015

davidlonjon commented Sep 9, 2021

github-actions bot commented Sep 9, 2021 •

edited

anomiex Sep 9, 2021

thingalon Sep 10, 2021

davidlonjon Sep 10, 2021

thingalon left a comment

thingalon left a comment

anomiex left a comment

davidlonjon commented Sep 14, 2021

	!projects/plugins/boost/app/assets/src/js/*.d.ts
	!projects/plugins/boost/app/assets/src/js/*/.d.ts

Boost: Fix linting config and standards in files #21015

Boost: Fix linting config and standards in files #21015

Conversation

davidlonjon commented Sep 9, 2021

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Sep 9, 2021 • edited

anomiex Sep 9, 2021

Choose a reason for hiding this comment

thingalon Sep 10, 2021

Choose a reason for hiding this comment

davidlonjon Sep 10, 2021

Choose a reason for hiding this comment

thingalon left a comment

Choose a reason for hiding this comment

thingalon left a comment

Choose a reason for hiding this comment

anomiex left a comment

Choose a reason for hiding this comment

davidlonjon commented Sep 14, 2021

github-actions bot commented Sep 9, 2021 •

edited