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

fix(providence): correct export path for MatchImportAnalyzer #2137

Merged
merged 4 commits into from
Nov 30, 2023
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
5 changes: 5 additions & 0 deletions .changeset/big-gifts-reflect copy 2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'providence-analytics': patch
---

feat: allow to resolve outside node_modules as well
5 changes: 5 additions & 0 deletions .changeset/big-gifts-reflect copy 3.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'providence-analytics': patch
---

feat: do not throw on unparseable files, but allow to proceed run for rest of projects
5 changes: 5 additions & 0 deletions .changeset/big-gifts-reflect copy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'providence-analytics': patch
---

fix: swc-traverse does not fail on object proto builtins like "toString"
5 changes: 5 additions & 0 deletions .changeset/big-gifts-reflect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'providence-analytics': patch
---

fix: corrected export path for MatchImportAnalyzer
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import path from 'path';
import { isRelativeSourcePath } from '../../utils/relative-source-path.js';
import { LogService } from '../../core/LogService.js';
import { resolveImportPath } from '../../utils/resolve-import-path.js';
Expand Down Expand Up @@ -35,7 +36,10 @@ export async function fromImportToExportPerspective({ importee, importer, import
return null;
}

const absolutePath = await resolveImportPath(importee, importer);
const absolutePath = await resolveImportPath(importee, importer, {
modulePaths: [path.resolve(importeeProjectPath, '..')],
});

if (!absolutePath) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export { Analyzer } from '../core/Analyzer.js';
// Expose analyzers that are requested to be run in external contexts
export { default as FindExportsAnalyzer } from './find-exports.js';
export { default as FindImportsAnalyzer } from './find-imports.js';
export { default as MatchImportsAnalyzer } from './match-paths.js';
export { default as MatchImportsAnalyzer } from './match-imports.js';

export { transformIntoIterableFindImportsOutput } from './helpers/transform-into-iterable-find-imports-output.js';
export { transformIntoIterableFindExportsOutput } from './helpers/transform-into-iterable-find-exports-output.js';
19 changes: 13 additions & 6 deletions packages-node/providence-analytics/src/program/core/Analyzer.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable no-param-reassign */
import semver from 'semver';
import pathLib from 'path';
import path from 'path';
import { LogService } from './LogService.js';
import { QueryService } from './QueryService.js';
import { ReportService } from './ReportService.js';
Expand Down Expand Up @@ -33,9 +33,17 @@ async function analyzePerAstFile(projectData, astAnalysis, analyzerCfg) {
for (const { file, ast, context: astContext } of projectData.entries) {
const relativePath = getFilePathRelativeFromRoot(file, projectData.project.path);
const context = { code: astContext.code, relativePath, projectData, analyzerCfg };
LogService.debug(`${pathLib.resolve(projectData.project.path, file)}`);
const { result, meta } = await astAnalysis(ast, context);
entries.push({ file: relativePath, meta, result });

const fullPath = path.resolve(projectData.project.path, file);
LogService.debug(`[analyzePerAstFile]: ${fullPath}`);

// We do a try and catch here, so that unparseable files do not block all metrics we're gathering in a run
try {
const { result, meta } = await astAnalysis(ast, context);
entries.push({ file: relativePath, meta, result });
} catch (e) {
LogService.error(`[analyzePerAstFile]: ${fullPath} throws: ${e}`);
}
}
const filteredEntries = entries.filter(({ result }) => Boolean(result.length));
return filteredEntries;
Expand Down Expand Up @@ -137,15 +145,14 @@ const checkForMatchCompatibility = (
/** @type {PathFromSystemRoot} */ referencePath,
/** @type {PathFromSystemRoot} */ targetPath,
) => {
// const refFile = pathLib.resolve(referencePath, 'package.json');
const referencePkg = InputDataService.getPackageJson(referencePath);
// const targetFile = pathLib.resolve(targetPath, 'package.json');
const targetPkg = InputDataService.getPackageJson(targetPath);

const allTargetDeps = [
...Object.entries(targetPkg?.devDependencies || {}),
...Object.entries(targetPkg?.dependencies || {}),
];

const importEntry = allTargetDeps.find(([name]) => referencePkg?.name === name);
if (!importEntry) {
return { compatible: false, reason: 'no-dependency' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ const fakePluginContext = {
* name without an extension.
* @param {SpecifierSource} importee source like '@lion/core' or '../helpers/index.js'
* @param {PathFromSystemRoot} importer importing file, like '/my/project/importing-file.js'
* @param {{customResolveOptions?: {preserveSymlinks:boolean}}} [opts] nodeResolve options
* @param {{customResolveOptions?: {preserveSymlinks:boolean}; modulePaths?: string[]}} [opts] nodeResolve options
* @returns {Promise<PathFromSystemRoot|null|'[node-builtin]'>} the resolved file system path, like '/my/project/node_modules/@lion/core/index.js'
*/
async function resolveImportPathFn(importee, importer, opts) {
async function resolveImportPathFn(importee, importer, opts = {}) {
if (isBuiltin(importee)) {
return '[node-builtin]';
}
Expand All @@ -49,7 +49,7 @@ async function resolveImportPathFn(importee, importer, opts) {
// allow resolving polyfills for nodejs libs
preferBuiltins: false,
// extensions: ['.mjs', '.js', '.json', '.node'],
...(opts || {}),
...opts,
});

const preserveSymlinks =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function createSwcPath(node, parent, stop, scope) {
*/
function isBindingNode(parent, identifierValue) {
if (parent.type === 'VariableDeclarator') {
// @ts-ignore
// @ts-expect-error
return parent.id.value === identifierValue;
}
return [
Expand All @@ -120,10 +120,7 @@ function isBindingNode(parent, identifierValue) {
}

/**
* Is the node:
* - a declaration (like "const a = 1")?
* - an import specifier (like "import { a } from 'b'")?
* Handy to know if the parents of Identifiers mark a binding
* Is the node a reference to a binding (like "alert(a);")?
* @param {SwcNode} parent
*/
function isBindingRefNode(parent) {
Expand Down Expand Up @@ -184,7 +181,8 @@ function addPotentialBindingOrRefToScope(swcPathForIdentifier) {
}
// In other cases, we are dealing with a reference that must be bound to a binding
else if (isBindingRefNode(parent)) {
const binding = scope.bindings[node.value];
// eslint-disable-next-line no-prototype-builtins
const binding = scope.bindings.hasOwnProperty(node.value) && scope.bindings[node.value];
if (binding) {
binding.refs.push(parentPath);
} else {
Expand Down Expand Up @@ -297,7 +295,7 @@ export function swcTraverse(swcAst, visitor, { needsAdvancedPaths = false } = {}
const newOrCurScope = getNewScope(swcPath, scope, traversalContext) || scope;
swcPath.scope = newOrCurScope;
addPotentialBindingOrRefToScope(swcPath);
return { newOrCurScope, swcPath };
return { swcPath, newOrCurScope };
};

/**
Expand Down Expand Up @@ -355,6 +353,6 @@ export function swcTraverse(swcAst, visitor, { needsAdvancedPaths = false } = {}
prepareTree(swcAst, null, initialScope, traversalContext);
}
visitTree(swcAst, null, initialScope, { hasPreparedTree: needsAdvancedPaths }, traversalContext);
// @ts-ignore
// @ts-expect-error
traversalContext.visitOnExitFns.reverse().forEach(fn => fn());
}
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ describe('swcTraverse', () => {
// TODO: also add case for Script
expect(rootPath.node.type).to.equal('Module');
});

it('does not fail on object prototype built-ins (like "toString")', async () => {
const code = `const { toString } = x;`;
const swcAst = await AstService._getSwcAst(code);

expect(swcTraverse(swcAst, {})).to.not.throw;
});
});

describe.skip('Paths', () => {
Expand Down
Loading