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

chore: migrate to eslint #7368

Merged
merged 2 commits into from Nov 20, 2023
Merged

chore: migrate to eslint #7368

merged 2 commits into from Nov 20, 2023

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Oct 29, 2023

This just removes tslint and its associated packages, replacing them with eslint and @typescript-eslint.

Any custom rule config we had has been discarded so we can instead rely on the recommended configs from both eslint and tseslint.

let me know if you want this. if you want any specific rules too, just let me know.

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

The only thing I saw in there is where we changed getArguments, which was literally trying to get an Arguments instance on purpose. I think if that's changed back, we're good to go with this.

I REALLY appreciate this PR.

@43081j
Copy link
Contributor Author

43081j commented Oct 30, 2023

no worries, i've reverted that and added an ignore.

we may have also found a bug in the prefer-const eslint rule, or just a limitation. in that anywhere we do this pattern:

let x;

doThingLaterOn(() => {
  x();
});
doThingNow(() => {
  x = whatever;
});

it tells us to use const but we can't ofc. I just ignored those lines

also, keep in mind the dev site still uses tslint for the tools. we can probably migrate away from that in a separate pr

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Oops. It looks like we have a failing test. Womp womp.

@43081j
Copy link
Contributor Author

43081j commented Oct 30, 2023

not sure why CI is failing... something to do with core-js in the dev site's build? 🤔 any idea?

@benlesh
Copy link
Member

benlesh commented Nov 3, 2023

@43081j I think if you rebase now, it may resolve the issue.

@43081j
Copy link
Contributor Author

43081j commented Nov 7, 2023

sorry for the delay @benlesh , have rebased now 👍

This just removes tslint and its associated packages, replacing them
with eslint and `@typescript-eslint`.

Any custom rule config we had has been discarded so we can instead rely
on the recommended configs from both eslint and tseslint.
@43081j
Copy link
Contributor Author

43081j commented Nov 13, 2023

rebased again fyi

@benlesh
Copy link
Member

benlesh commented Nov 20, 2023

The failure here seems weird. I'm going to go ahead and merge this, and sort it out separately.

@benlesh benlesh merged commit bbb685f into ReactiveX:master Nov 20, 2023
1 of 3 checks passed
@benlesh
Copy link
Member

benlesh commented Nov 20, 2023

Thank you again, @43081j ... this is a very helpful PR

@43081j 43081j deleted the eslint branch November 20, 2023 17:51
@43081j
Copy link
Contributor Author

43081j commented Nov 20, 2023

no problem, thanks for taking time to review

i also couldn't figure the failure out since i couldn't repro it locally, will be interesting to find out what it was 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants