-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Addresses #10 / Update semver
to the latest to fix CVE / Remove unnecessary dependencies
#11
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## fork-release #11 +/- ##
================================================
- Coverage 95.02% 94.76% -0.26%
================================================
Files 68 68
Lines 2977 2982 +5
Branches 1028 1028
================================================
- Hits 2829 2826 -3
- Misses 148 156 +8
☔ View full report in Codecov by Sentry. |
Ops, it seems that I forgot to remove those outdated Node.js versions from the CI matrix! |
What benefits would we get from this do you think? |
|
Can we drop UPD: Maybe we can't :( |
@so1ve Actually maybe we can! The https://nodejs.org/dist/latest-v18.x/docs/api/module.html#modulebuiltinmodules ![]() |
I'm not so sure for these, actually I was thinking to maintain this package separately from the upstream. As you said, such changes make rebasing a bit complicated, if we do so it would mean we're really start "forking", but it would mean a lot of extra efforts and hurt the community I think. But of course, I would like to listen from the community, I was trying to support the same node versions as upstream but it's also boring and "ridiculous". So I like such changes personally, but also I'm in awe of this impactful project. Let's wait for some more feedback for a while. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Additionally, less dependencies may reduce possible supply-chain attacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the idea of reducing unnecessary dependencies.
You can replace |
Although ljharb hasn't been actively maintaining I have rebased the PR and minimized the affected lines to ensure that there will be least merging conflicts in the future. Update I also move |
semver
to the latest to fix CVE / Remove unnecessary dependencies
I have bumped the
All test cases pass on my machine, waiting for the CI. |
Thanks, lets make it happen. |
Let's drop node 13 15 17 19 |
We also need to update the Node.js versions in appveyor config. |
I guess we can delete that file because we don't use appveyor at all |
I don't know. IMHO the PR has contained enough changes. @JounQin What do you think? |
It's not used, then no change is needed. |
Co-authored-by: JounQin <admin@1stg.me>
Co-authored-by: JounQin <admin@1stg.me>
For anyone may be interested, I just upgraded and I don't have enough time to work on this replacement, anyone interested can help to raise a PR which will be appreciated. For import-js#2900 (comment), I think it should be changed to Tracking this replacement at #22 |
The PR does the following things:
is-core-module
withrequire('module').Module.builtinModules
, replacehas
withObject.prototype.hasOwnProperty.call
dependencies
is better: Addresses #10 / Updatesemver
to the latest to fix CVE / Remove unnecessary dependencies #11 (comment)semver
to the latest versionsemver
to fix vulnerability import-js/eslint-plugin-import#2806eslint-plugin-i
is a fork ofeslint-plugin-import
that replaces thetsconfig-paths
with the faster and more lightweightget-tsconfig
, which also addresses the following issues:If you want to replace
eslint-plugin-import
witheslint-plugin-i
, just run following command in your projects: