Skip to content

Conversation

@wemeetagain
Copy link
Member

Description

Use local defintion of isPrivateIp instead of private-ip package
This should reduce bundle size and remove another commonjs sub-dependency (ipaddr.js)

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@wemeetagain wemeetagain requested a review from a team as a code owner January 16, 2024 17:42
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

You should verify that this decreases the bundle size and update the limits as well for each package so that this change is worthwhile.

@@ -0,0 +1,203 @@
/* eslint-env mocha */
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, also the source is a modified version of https://github.com/frenchbread/private-ip/blob/master/src/index.ts

@wemeetagain
Copy link
Member Author

You should verify that this decreases the bundle size

Its guaranteed to be smaller because its the same code, but with two fewer dependencies.

@achingbrain
Copy link
Member

@wemeetagain LGTM but CI is failing, could you take a look please?

@wemeetagain
Copy link
Member Author

@achingbrain ready for re-review

@achingbrain achingbrain changed the title feat: add local definition of isPrivateIp fix: add local definition of isPrivateIp Jan 18, 2024
@achingbrain achingbrain merged commit f27138c into main Jan 18, 2024
@achingbrain achingbrain deleted the feat/private-ip branch January 18, 2024 18:25
@achingbrain achingbrain mentioned this pull request Jan 18, 2024
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.

5 participants