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

Reduce number of dependencies #2628

Closed
wants to merge 3 commits into from
Closed

Conversation

benmccann
Copy link

@benmccann benmccann commented Jun 21, 2024

deep-equal is a terribly heavy library: https://npmgraph.js.org/?q=@turf/helpers

also updates to pnpm 9 and updates the versions tested on the CI accordingly

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

Submitting a new TurfJS Module.

  • Overview description of proposed module.
  • Include JSDocs with a basic example.
  • Execute ./scripts/generate-readmes to create README.md.
  • Add yourself to contributors in package.json using "Full Name <@github Username>".

Screenshot from 2024-06-21 07-53-10

@smallsaucepan
Copy link
Member

Thanks for putting the work into this @benmccann. Removing deep-equal from turf-helpers is currently underway in #2623. Once that's merged would you be happy to pull those changes into here and then we can convert any remaining instance/s (turf-line-overlap only?)?

Also noticed changes to the versions of node we build Turf against and other workflow changes. All for keeping things modern. Could you please make mention in the description though the PR includes this?

@@ -69,7 +68,7 @@
"typescript": "^5.2.2"
},
"dependencies": {
"deep-equal": "^2.2.3",
"dequal": "^2.0.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider fast-deep-equal at all? I hadn't heard of dequal but it seems plenty popular. I didn't compare final bundle sizes with both, but they're both much smaller than deep-equals

Copy link
Author

Choose a reason for hiding this comment

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

they both seem pretty good. I don't really have much of a preference for one vs the other

with:
version: 8
version: 9
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would strongly prefer that the build tooling changes go into another PR. Technically removing supported node versions is a break, but also 16 is EOL'd. We might consider just adding 22 and not removing 16 unless we want to 'sneak' that break into 7.x.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, makes sense. I just only have pnpm 9 installed on my machine and unfortunately they've dropped support for older node versions

if a maintainer would like to grab any changes you'd like to keep from this PR and send it, please feel free to do so

#2623 is already accomplishing most of what I was aiming for, so I'd also be happy to close this one if that's easier

@mfedderly
Copy link
Collaborator

Ok I opened #2631 to remove deep-equals from the other package that had included it. We need to merge #2623 first and then I'll fix #2631 up so that we remove the dependency everywhere. If you feel strongly that we should make the workflow changes lets continue that here. Otherwise feel free to close this.

@benmccann
Copy link
Author

thank you!!

@benmccann benmccann closed this Jun 24, 2024
@benmccann benmccann deleted the dequal branch June 24, 2024 21:52
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