Skip to content

refactor: remove unused dependencies #1905

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

Closed

Conversation

damianstasik
Copy link
Contributor

@damianstasik damianstasik commented Oct 31, 2021

New Pull Request Checklist

Issue Description

Both typescript and babel-plugin-transform-object-rest-spread are not used anywhere, there is no need to keep them.

Related issue: #1902

Approach

TODOs before merging

  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 31, 2021

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@@ -71,8 +71,7 @@
"react-router": "5.2.1",
"react-router-dom": "5.3.0",
"regenerator-runtime": "0.13.8",
"semver": "7.3.4",
"typescript": "4.4.3"
Copy link
Member

Choose a reason for hiding this comment

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

typescript is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtrezza could you specify what is using typescript? I checked the repo and there is no configuration for TS, there are no .tsx? files and no build set up for TS 🤔

Copy link
Member

@mtrezza mtrezza Oct 31, 2021

Choose a reason for hiding this comment

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

madge uses it; even if you don't see any reference for a dependency in the code base, another dependency may require it based on its configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtrezza I checked the report madge generates and everything works.

Screen Shot 2021-11-01 at 16 28 49

even if you don't see any reference for a dependency in the code base, another dependency may require it based on its configuration.

Yeah, I get that, I wouldn't try to send the PR otherwise. I checked peer deps of other packages before to be sure if TS is really not required.

Copy link
Member

@mtrezza mtrezza Nov 1, 2021

Choose a reason for hiding this comment

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

image

It's a dependency N levels deep, you'd have look at package-lock.json from madge to see that, you won't see that in its package.json. I'm assuming it's configuration-dependent in madge, so it's probably not used in all cases, that's why it may be not a fixed dependency of the madge package.

@mtrezza
Copy link
Member

mtrezza commented Oct 31, 2021

Could you please resolve the conflict?

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.

3 participants