-
Notifications
You must be signed in to change notification settings - Fork 181
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
JS-493 Add 'get-telemetry' endpoint on bridge-server #5011
Changes from 6 commits
df0cfa0
a6627bf
4618133
862fd75
6468e13
fcd1445
7e69263
a41db43
7a50945
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,10 +30,37 @@ const findPackageJsons = createFindUp(PACKAGE_JSON); | |
|
||
const DefinitelyTyped = '@types/'; | ||
|
||
type MinimatchDependency = { | ||
name: Minimatch; | ||
version?: string; | ||
}; | ||
|
||
export type NamedDependency = { | ||
name: string; | ||
version?: string; | ||
}; | ||
|
||
type Dependency = MinimatchDependency | NamedDependency; | ||
|
||
/** | ||
* Cache for the available dependencies by dirname. | ||
*/ | ||
const cache: Map<string, Set<string | Minimatch>> = new Map(); | ||
const cache: Map<string, Set<Dependency>> = new Map(); | ||
|
||
export function getAllDependencies(): NamedDependency[] { | ||
const dependencies = [...cache.values()] | ||
.flatMap(dependencies => [...dependencies]) | ||
.filter((dependency): dependency is NamedDependency => typeof dependency.name === 'string'); | ||
return Object.values( | ||
dependencies.reduce( | ||
(result, dependency) => ({ | ||
...result, | ||
[dependency.name]: dependency, | ||
}), | ||
{}, | ||
), | ||
); | ||
Comment on lines
+59
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this step doing? Since we already have a NemdDependency[] at the previous step. |
||
} | ||
|
||
/** | ||
* Retrieve the dependencies of all the package.json files available for the given file. | ||
|
@@ -46,9 +73,9 @@ export function getDependencies(filename: string, cwd: string) { | |
const dirname = Path.dirname(toUnixPath(filename)); | ||
const cached = cache.get(dirname); | ||
if (cached) { | ||
return cached; | ||
return new Set([...cached].map(item => item.name)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this doing? Just a clone or something more? |
||
} | ||
const result = new Set<string | Minimatch>(); | ||
const result = new Set<Dependency>(); | ||
cache.set(dirname, result); | ||
|
||
getManifests(dirname, cwd, fs).forEach(manifest => { | ||
|
@@ -59,7 +86,7 @@ export function getDependencies(filename: string, cwd: string) { | |
}); | ||
}); | ||
|
||
return result; | ||
return new Set([...result].map(item => item.name)); | ||
} | ||
|
||
/** | ||
|
@@ -71,7 +98,7 @@ export function clearDependenciesCache() { | |
} | ||
|
||
export function getDependenciesFromPackageJson(content: PackageJson) { | ||
const result = new Set<string | Minimatch>(); | ||
const result = new Set<Dependency>(); | ||
if (content.name) { | ||
addDependencies(result, { [content.name]: '*' }); | ||
} | ||
|
@@ -99,30 +126,37 @@ export function getDependenciesFromPackageJson(content: PackageJson) { | |
} | ||
|
||
function addDependencies( | ||
result: Set<string | Minimatch>, | ||
result: Set<Dependency>, | ||
dependencies: PackageJson.Dependency, | ||
isGlob = false, | ||
) { | ||
Object.keys(dependencies).forEach(name => addDependency(result, name, isGlob)); | ||
Object.keys(dependencies).forEach(name => | ||
addDependency(result, name, isGlob, dependencies[name]), | ||
); | ||
} | ||
|
||
function addDependenciesArray( | ||
result: Set<string | Minimatch>, | ||
dependencies: string[], | ||
isGlob = true, | ||
) { | ||
function addDependenciesArray(result: Set<Dependency>, dependencies: string[], isGlob = true) { | ||
dependencies.forEach(name => addDependency(result, name, isGlob)); | ||
} | ||
|
||
function addDependency(result: Set<string | Minimatch>, dependency: string, isGlob: boolean) { | ||
function addDependency( | ||
result: Set<Dependency>, | ||
dependency: string, | ||
isGlob: boolean, | ||
version?: string, | ||
) { | ||
if (isGlob) { | ||
result.add(new Minimatch(dependency, { nocase: true, matchBase: true })); | ||
result.add({ | ||
name: new Minimatch(dependency, { nocase: true, matchBase: true }), | ||
version, | ||
}); | ||
} else { | ||
result.add( | ||
dependency.startsWith(DefinitelyTyped) | ||
result.add({ | ||
name: dependency.startsWith(DefinitelyTyped) | ||
? dependency.substring(DefinitelyTyped.length) | ||
: dependency, | ||
); | ||
version, | ||
}); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,9 @@ import { jsTsInput, parseJavaScriptSourceFile } from '../tools/index.js'; | |
import { Linter, Rule } from 'eslint'; | ||
import { describe, beforeEach, it } from 'node:test'; | ||
import { expect } from 'expect'; | ||
import { getManifests, toUnixPath } from '../../src/rules/helpers/index.js'; | ||
import { getDependencies, getManifests, toUnixPath } from '../../src/rules/helpers/index.js'; | ||
import { setContext } from '../../../shared/src/helpers/context.js'; | ||
import { analyzeJSTS } from '../../src/analysis/analyzer.js'; | ||
import { analyzeJSTS, getTelemetry } from '../../src/analysis/analyzer.js'; | ||
import { APIError } from '../../../shared/src/errors/error.js'; | ||
import { RuleConfig } from '../../src/linter/config/rule-config.js'; | ||
import { initializeLinter } from '../../src/linter/linters.js'; | ||
|
@@ -899,6 +899,50 @@ describe('analyzeJSTS', () => { | |
expect(vueIssues[0].message).toEqual('call'); | ||
}); | ||
|
||
it('should populate dependencies after analysis', async () => { | ||
const baseDir = path.join(currentPath, 'fixtures', 'dependencies'); | ||
const linter = new Linter(); | ||
linter.defineRule('custom-rule-file', { | ||
create(context) { | ||
return { | ||
CallExpression(node) { | ||
// Necessarily call 'getDependencies' to populate the cache of dependencies | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that we would not receive any dependencies in the telemetry in case the analysis never populated the cache? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a good chance that this happens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we will only have dependencies once the analysis populates it. Chance is low. I guess one would have to remove all of the rules that call the package-jsons.ts cache. In that case, we wouldn't populate anything for them. That's a tradeoff. This way, this call is very cheap and doesn't incur an extra time-cost. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and could you document this behavior somewhere, preferably in the code base like in the JS doc of the getDependencies() function:
|
||
const dependencies = getDependencies(toUnixPath(context.filename), baseDir); | ||
if (dependencies.size) { | ||
context.report({ | ||
node: node.callee, | ||
message: 'call', | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
} as Rule.RuleModule); | ||
const filePath = path.join(currentPath, 'fixtures', 'dependencies', 'index.js'); | ||
const sourceCode = await parseJavaScriptSourceFile(filePath); | ||
linter.verify( | ||
sourceCode, | ||
{ rules: { 'custom-rule-file': 'error' } }, | ||
{ filename: filePath, allowInlineConfig: false }, | ||
); | ||
const telemetry = getTelemetry(); | ||
const dependencies = telemetry.dependencies; | ||
zglicz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(dependencies).toStrictEqual([ | ||
{ | ||
name: 'test-module', | ||
version: '*', | ||
}, | ||
{ | ||
name: 'pkg1', | ||
version: '1.0.0', | ||
}, | ||
{ | ||
name: 'pkg2', | ||
version: '2.0.0', | ||
}, | ||
]); | ||
}); | ||
|
||
it('should return the AST along with the issues', async () => { | ||
const rules = [{ key: 'S4524', configurations: [], fileTypeTarget: ['MAIN'] }] as RuleConfig[]; | ||
await initializeLinter(rules); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
console.log("Hello World!"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"name": "test-module", | ||
"version": "1.0.0", | ||
"author": "Your Name <email@example.com>", | ||
"dependencies": { | ||
"pkg1": "1.0.0" | ||
}, | ||
"devDependencies": { | ||
"pkg2": "2.0.0" | ||
} | ||
} |
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.
if we have duplicates, would it simply overwrite and keep the last one?