Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Fix plugins not being found when no node_modules exists #171

Merged
merged 3 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"debug": "^4.1.1",
"globby": "^11.0.1",
"is-wsl": "^2.1.1",
"resolve": "^1.17.0",
"tslib": "^2.0.0"
},
"devDependencies": {
Expand All @@ -19,6 +20,7 @@
"@types/mocha": "^8.0.0",
"@types/node": "^14.0.14",
"@types/proxyquire": "^1.3.28",
"@types/resolve": "^1.17.1",
"@types/wrap-ansi": "^3.0.0",
"chai": "^4.2.0",
"conventional-changelog-cli": "^2.0.21",
Expand Down
54 changes: 23 additions & 31 deletions src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {Manifest} from './manifest'
import {PJSON} from './pjson'
import {Topic} from './topic'
import {tsPath} from './ts-node'
import {compact, exists, flatMap, loadJSON, mapValues} from './util'
import {compact, exists, resolvePackage, flatMap, loadJSON, mapValues} from './util'

const ROOT_INDEX_CMD_ID = ''

Expand Down Expand Up @@ -98,40 +98,32 @@ function topicsToArray(input: any, base?: string): Topic[] {
})
}

// eslint-disable-next-line valid-jsdoc
/**
* find package root
* for packages installed into node_modules this will go up directories until
* it finds a node_modules directory with the plugin installed into it
*
* This is needed because of the deduping npm does
*/
async function findRoot(name: string | undefined, root: string) {
// essentially just "cd .."
function * up(from: string) {
while (path.dirname(from) !== from) {
yield from
from = path.dirname(from)
}
// essentially just "cd .."
function * up(from: string) {
while (path.dirname(from) !== from) {
yield from
from = path.dirname(from)
}
yield from
}

async function findSourcesRoot(root: string) {
for (const next of up(root)) {
let cur
if (name) {
cur = path.join(next, 'node_modules', name, 'package.json')
// eslint-disable-next-line no-await-in-loop
if (await exists(cur)) return path.dirname(cur)
try {
// eslint-disable-next-line no-await-in-loop
const pkg = await loadJSON(path.join(next, 'package.json'))
if (pkg.name === name) return next
} catch { }
} else {
cur = path.join(next, 'package.json')
// eslint-disable-next-line no-await-in-loop
if (await exists(cur)) return path.dirname(cur)
}
const cur = path.join(next, 'package.json')
// eslint-disable-next-line no-await-in-loop
if (await exists(cur)) return path.dirname(cur)
}
}

async function findRoot(name: string | undefined, root: string) {
if (name) {
let pkgPath
try {
pkgPath = await resolvePackage(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to pass in { basedir: root } because it may need to look outside of the current path. i.e. where user plugins are stored.

With that said, the resolve library isn't working how I thought it would. If I add the basedir above, it will still sometimes throw MODULE_NOT_FOUND even though it seems to be in the resolution path.

For example, I modified resolvePackage to take in the same options object that resolve takes and pass that to resolve.

// name = '@salesforce/plugin-evergreen-build'
// root = '/Users/t.dvornik/.local/share/sfdx/node_modules/@salesforce/plugin-evergreen'
...
// The root may not always be a dir, so ensure it is before passing it into resolve
if (path.extname(root)) {
  root = path.dirname(root)
}
try {
  pkgPath = await util_2.resolvePackage(name, { basedir: root });
}
...

That package is in /Users/t.dvornik/.local/share/sfdx/node_modules/@salesforce/plugin-evergreen-build which should be in node module resolution but resolve isn't resolving it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at this - this issue flew under the radar for me for a minute, but I'll have a look to see if I can fix it sometime soon. It's not high priority for me but evidently there's a few others others also waiting on a fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we don't just use the native require.resolve over the resolve package? it doesn't appear that we really need it to resolve asynchronously for our use case.

} catch (error) {}
return pkgPath ? findSourcesRoot(path.dirname(pkgPath)) : pkgPath
}
return findSourcesRoot(root)
}

export class Plugin implements IPlugin {
Expand Down
12 changes: 12 additions & 0 deletions src/util.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as fs from 'fs'
import * as _resolve from 'resolve'

const debug = require('debug')('@oclif/config')

Expand All @@ -18,6 +19,17 @@ export function exists(path: string): Promise<boolean> {
return new Promise(resolve => resolve(fs.existsSync(path)))
}

export function resolvePackage(id: string): Promise<string> {
return new Promise(
(resolve, reject) =>
_resolve(
id,
(err, pkgPath) =>
err ? reject(err) : resolve(pkgPath),
),
)
}

export function loadJSON(path: string): Promise<any> {
debug('loadJSON %s', path)
// let loadJSON
Expand Down
53 changes: 53 additions & 0 deletions test/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import * as path from 'path'

import {expect, fancy} from './test'
import * as Plugin from '../src/plugin'
import * as util from '../src/util'

const root = path.resolve(__dirname, 'fixtures/typescript')
const pluginName = '@oclif/plugin-help'
const pluginLocation = 'some/external/directory'
const pluginPjsonLocation = path.join(pluginLocation, 'package.json')

const withPluginInstance = () => {
return fancy
.add('plugin', () => new Plugin.Plugin({
type: 'core',
root,
name: pluginName,
ignoreManifest: true,
}))
.stub(util, 'exists', (checkPath: string) => checkPath === pluginPjsonLocation)
.stub(util, 'resolvePackage', (id: string): Promise<string> => {
if (id !== pluginName) {
return Promise.reject()
}
return Promise.resolve(path.join(pluginLocation, 'lib', 'index.js'))
})
.stub(util, 'loadJSON', (jsonPath: string) => {
if (jsonPath !== pluginPjsonLocation) {
return {}
}
return {
name: pluginName,
version: '1.0.0',
files: [],
oclif: {},
}
})
}

describe('Plugin', () => {
withPluginInstance()
.it('Should correctly instantiate a Plugin instance', ctx => {
expect(ctx.plugin).to.be.instanceOf(
Plugin.Plugin,
'Expected instance to be an instance of Plugin!',
)
})

withPluginInstance()
.it('Should correctly load a Plugin', async ctx => {
await ctx.plugin.load()
})
})
9 changes: 8 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,13 @@
resolved "https://registry.yarnpkg.com/@types/proxyquire/-/proxyquire-1.3.28.tgz#05a647bb0d8fe48fc8edcc193e43cc79310faa7d"
integrity sha512-SQaNzWQ2YZSr7FqAyPPiA3FYpux2Lqh3HWMZQk47x3xbMCqgC/w0dY3dw9rGqlweDDkrySQBcaScXWeR+Yb11Q==

"@types/resolve@^1.17.1":
version "1.17.1"
resolved "https://registry.yarnpkg.com/@types/resolve/-/resolve-1.17.1.tgz#3afd6ad8967c77e4376c598a82ddd58f46ec45d6"
integrity sha512-yy7HuzQhj0dhGpD8RLXSZWEkLsV9ibvxvi6EiJ3bkqLAO1RGo0WbkWQiwpRlSFymTJRz0d3k5LM3kkx8ArDbLw==
dependencies:
"@types/node" "*"

"@types/sinon@*":
version "7.0.11"
resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-7.0.11.tgz#6f28f005a36e779b7db0f1359b9fb9eef72aae88"
Expand Down Expand Up @@ -2496,7 +2503,7 @@ resolve-from@^4.0.0:
resolved "https://registry.yarnpkg.com/resolve-from/-/resolve-from-4.0.0.tgz#4abcd852ad32dd7baabfe9b40e00a36db5f392e6"
integrity sha512-pb/MYmXstAkysRFx8piNI1tGFNQIFA3vkE3Gq4EuA1dF6gHp/+vgZqsCGJapvy8N3Q+4o7FwvquPJcnZ7RYy4g==

resolve@^1.1.6, resolve@^1.10.0, resolve@^1.11.1, resolve@^1.8.1:
resolve@^1.1.6, resolve@^1.10.0, resolve@^1.11.1, resolve@^1.17.0, resolve@^1.8.1:
version "1.17.0"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.17.0.tgz#b25941b54968231cc2d1bb76a79cb7f2c0bf8444"
integrity sha512-ic+7JYiV8Vi2yzQGFWOkiZD5Z9z7O2Zhm9XMaTxdJExKasieFCr+yXZ/WmXsckHiKl12ar0y6XiXDx3m4RHn1w==
Expand Down