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

feat: speed up eslint parser timing #3250

Merged
merged 5 commits into from
Apr 20, 2023
Merged

feat: speed up eslint parser timing #3250

merged 5 commits into from
Apr 20, 2023

Conversation

scopsy
Copy link
Contributor

@scopsy scopsy commented Apr 20, 2023

What change does this PR introduce?

This PR introduces a performance improvement to eslint by localizing the ts parser options to the scanned object rather than the whole repo from the root. This change resulted in a significant reduction in test case running time, from 20s to 5s. Additionally, cspell has been removed from eslint, as it caused more than 70% of the time spent when linting.

Why was this change needed?

This change was needed to improve the performance of eslint, which was previously slow due to the ts parser options being applied to the entire repo from the root. This caused husky pre commit take a forever amount of time to complete

Before:
CleanShot 2023-04-20 at 09 40 04@2x

After:
CleanShot 2023-04-20 at 09 39 35@2x

Cspell took almost all the time, now we will only run it in CI.
CleanShot 2023-04-20 at 09 42 51@2x

@@ -7,7 +7,6 @@ module.exports = {
'prettier',
'plugin:prettier/recommended',
'plugin:promise/recommended',
'plugin:@cspell/recommended',
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove .cspell.json file too. Or are we keeping it in the CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p-fernandez keeping it in CI, just making sure locally our linter will run faster

@@ -37,7 +37,6 @@ export class InviteMember {
if (process.env.NOVU_API_KEY && (process.env.NODE_ENV === 'dev' || process.env.NODE_ENV === 'production')) {
const novu = new Novu(process.env.NOVU_API_KEY);

// eslint-disable-next-line @cspell/spellchecker
// cspell:disable-next
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove all the ocurrences of cspell anotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second annotation helps the CI skip this one. I only removed the eslint one here

@github-actions github-actions bot added @novu/api documentation Improvements or additions to documentation labels Apr 20, 2023
@scopsy scopsy merged commit 14a676a into next Apr 20, 2023
23 checks passed
@scopsy scopsy deleted the speed-up-linter-speed branch April 20, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation @novu/api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants