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

[Refactor] switch to an internal replacement for pkg-up and read-pkg-up #2047

Merged
merged 2 commits into from
Sep 18, 2021

Conversation

mgwalker
Copy link
Contributor

@mgwalker mgwalker commented May 6, 2021

Inspired by #2046, I dug into what read-pkg-up provides and found it to be very simple, for this particular case. Its downstream dependencies build in some safety and options that aren't necessary when you already know you're working with a package.json, and in this library's case, when you know you don't want to "normalize" it. Since read-pkg-up drags several libraries along with it, at least two of which don't actually add any functionality to eslint-plugin-import, and its use here is pretty straightforward, it seemed moderately reasonable to me to reproduce the functionality internally.

@coveralls
Copy link

coveralls commented May 6, 2021

Coverage Status

Coverage increased (+14.7%) to 84.198% when pulling e3a44f8 on mgwalker:remove-read-pkg-up into e871a9a on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'm not super stoked on copy-pasting, but removing this dep is valuable.

utils/readPkgUp.js Show resolved Hide resolved
utils/readPkgUp.js Outdated Show resolved Hide resolved
@silverwind
Copy link
Contributor

The replacement find-up@2.1.0 is very out of date and contains security vulnerabilites to boot. I'd say drop the dependency completely and replace it with a small function:

const {dirname} = require('path');
const {accessSync} = require('fs');

function findUpSync(dir, file) {
  const path = join(dir, file);
  try {
    accessSync(path);
    return path;
  } catch {}
  const parent = dirname(dir);
  return parent === dir ? null : findSync(parent, file);
}

Use via findUpSync(filePath, 'package.json').

@mgwalker
Copy link
Contributor Author

mgwalker commented May 7, 2021

Ergh, sorry for the failing tests. I didn't realize that eslint-module-utils was a published package as well as a local package. I haven't had a chance to dig into it very far yet, but my working assumption is that something changed in npm's order of operations when a dependency is listed in both dependencies and devDependencies. In npm 5.x and 6.x, it seems npm install will fetch from the published package if available. Since that one doesn't include the updates in this PR, the tests fail. In later versions of npm, it uses the local version instead, and everything is fine.

My apologies if this is known issue and there's an established workflow for updating eslint-module-utils separately from eslit-plugin-import.

@ljharb
Copy link
Member

ljharb commented May 7, 2021

There isn’t; you could make the changes in two PRs, and we could land and publish the utils one first?

altho i wouldn’t expect that to be needed - we use linklocal in this repo so everything should be using the latest local code.

@mgwalker
Copy link
Contributor Author

mgwalker commented May 7, 2021

Oh, well in that case, maybe just switching the dependencies to the local package will do the job. Pushed a new commit.

@mgwalker
Copy link
Contributor Author

mgwalker commented May 7, 2021

Also as you mentioned, the CVE got updated, so I guess closing this PR as moot would also make sense. 🤷🏻‍♂️

package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented May 14, 2021

Another alternative from a bit ago is #1017.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

went ahead and replaced pkg-up as well, and rebased. this should be good to go.

@ljharb ljharb changed the title Remove dependency on read-pkg-up [Refactor] switch to an internal replacement for pkg-up and read-pkg-up Sep 17, 2021
@ljharb ljharb added the package: utils eslint-module-utils package label Sep 17, 2021
@ljharb ljharb marked this pull request as ready for review September 17, 2021 23:19
@codecov
Copy link

codecov bot commented Sep 18, 2021

Codecov Report

Merging #2047 (9ccdcb7) into main (7c382f0) will increase coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2047      +/-   ##
==========================================
+ Coverage   84.02%   84.03%   +0.01%     
==========================================
  Files          94       96       +2     
  Lines        3017     3032      +15     
  Branches      898      899       +1     
==========================================
+ Hits         2535     2548      +13     
- Misses        482      484       +2     
Impacted Files Coverage Δ
utils/readPkgUp.js 81.81% <81.81%> (ø)
src/core/packagePath.js 80.00% <100.00%> (ø)
src/rules/no-extraneous-dependencies.js 100.00% <100.00%> (ø)
src/rules/no-import-module-exports.js 100.00% <100.00%> (ø)
src/rules/no-relative-packages.js 95.00% <100.00%> (ø)
src/rules/no-unused-modules.js 97.43% <100.00%> (ø)
utils/pkgUp.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c382f0...9ccdcb7. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: utils eslint-module-utils package
Development

Successfully merging this pull request may close these issues.

4 participants