Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

feat: Add migration comment transform #27

Merged
merged 2 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ You can run `npx @sentry/migr8 --help` to get a list of available options.

## Transformations

### Add migration comments

There are certain things that migr8 cannot auto-fix for you. This is the case for things like `startTransaction()`,
which cannot be replaced 1:1. This transform will try to identify the most common of these scenarios, and put comments
into your code in places where you need to do something manually.

### Use functional integrations instead of integration classes

This updates usage of class-based integrations to the new, functional style. For example:
Expand Down
116 changes: 116 additions & 0 deletions src/transformers/addMigrationComments/addMigrationComments.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import { afterEach, describe, it } from 'node:test';
import * as assert from 'node:assert';
import { rmSync } from 'node:fs';

import { getDirFileContent, getFixturePath, makeTmpDir } from '../../../test-helpers/testPaths.js';
import { assertStringEquals } from '../../../test-helpers/assert.js';

import tracingConfigTransformer from './index.js';

describe('transformers | tracingConfig', () => {
let tmpDir = '';

afterEach(() => {
if (tmpDir) {
rmSync(tmpDir, { force: true, recursive: true });
tmpDir = '';
}
});

it('has correct name', () => {
assert.equal(tracingConfigTransformer.name, 'Add migration comments');
});

it('works with app without Sentry', async () => {
tmpDir = makeTmpDir(getFixturePath('noSentry'));
await tracingConfigTransformer.transform([tmpDir], { filePatterns: [] });

const actual1 = getDirFileContent(tmpDir, 'app.js');
assert.equal(actual1, getDirFileContent(`${process.cwd()}/test-fixtures/noSentry`, 'app.js'));
});

it('works with startTransaction', async () => {
tmpDir = makeTmpDir(getFixturePath('addMigrationComments'));
const file = 'startTransaction.js';
const files = [`${tmpDir}/${file}`];
await tracingConfigTransformer.transform(files, { filePatterns: [] });

const actual = getDirFileContent(tmpDir, file);

assertStringEquals(
actual,
`import * as Sentry from '@sentry/browser';
import { startTransaction } from '@sentry/browser';

function doSomething() {
// TODO(sentry): Use \`startInactiveSpan()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md
startTransaction();
}

function doSomethingElse() {
// TODO(sentry): Use \`startInactiveSpan()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md
const transaction = Sentry.startTransaction();
transaction.finish();

const obj = {
// TODO(sentry): Use \`startInactiveSpan()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md
transaction: Sentry.startTransaction(),
};
}`
);
});

it('works with span.startChild', async () => {
tmpDir = makeTmpDir(getFixturePath('addMigrationComments'));
const file = 'startChild.js';
const files = [`${tmpDir}/${file}`];
await tracingConfigTransformer.transform(files, { filePatterns: [] });

const actual = getDirFileContent(tmpDir, file);

assertStringEquals(
actual,
`function doSomething(span) {
// TODO(sentry): Use \`startInactiveSpan()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md
span.startChild();
}

function doSomethingElse() {
const transaction = Sentry.getActiveSpan();
// TODO(sentry): Use \`startInactiveSpan()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md
transaction.startChild().end();
transaction.finish();
}`
);
});

it('works with hub', async () => {
tmpDir = makeTmpDir(getFixturePath('addMigrationComments'));
const file = 'hub.js';
const files = [`${tmpDir}/${file}`];
await tracingConfigTransformer.transform(files, { filePatterns: [] });

const actual = getDirFileContent(tmpDir, file);

assertStringEquals(
actual,
`import * as Sentry from '@sentry/browser';
import { makeMain, Hub } from '@sentry/browser';

function doSomething() {
// TODO(sentry): Use \`new Scope()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md
const hub = new Hub();
// TODO(sentry): Use \`setCurrentClient()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md
makeMain(hub);
}

function doSomethingElse() {
// TODO(sentry): Use \`new Scope()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md
const hub = new Sentry.Hub();
// TODO(sentry): Use \`setCurrentClient()\` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md
Sentry.makeMain(hub);
}
`
);
});
});
16 changes: 16 additions & 0 deletions src/transformers/addMigrationComments/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import path from 'path';
import url from 'url';

import { runJscodeshift } from '../../utils/jscodeshift.js';

/**
* @type {import('types').Transformer}
*/
export default {
name: 'Add migration comments',
transform: async (files, options) => {
const transformPath = path.join(path.dirname(url.fileURLToPath(import.meta.url)), './transform.cjs');

await runJscodeshift(transformPath, files, options);
},
};
121 changes: 121 additions & 0 deletions src/transformers/addMigrationComments/transform.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
const { wrapJscodeshift } = require('../../utils/dom.cjs');

/**
* Adds comments for migrations that migr8 can't do automatically.
* This helps the user to identify places where they need to do manual work.
*
* @type {import('jscodeshift').Transform}
*/
module.exports = function (fileInfo, api) {
const j = api.jscodeshift;
const source = fileInfo.source;
const fileName = fileInfo.path;

return wrapJscodeshift(j, source, fileName, (j, source) => {
const tree = j(source);

/** @type {Record<string,string>} */
const methodCommentMap = {
startTransaction:
'Use `startInactiveSpan()` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md',
startChild:
'Use `startInactiveSpan()` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-new-performance-apis.md',
makeMain:
'Use `setCurrentClient()` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md',
};

// Find `xxx()` calls
tree
.find(j.CallExpression, {
callee: {
type: 'Identifier',
},
})
.forEach(path => {
if (path.value.callee.type === 'Identifier') {
const comment = methodCommentMap[path.value.callee.name];
if (comment) {
addTodoComment(j, path, comment);
}
}
});

// Find `Sentry.xxx()` calls
tree
.find(j.CallExpression, {
callee: {
type: 'MemberExpression',
property: {
type: 'Identifier',
},
},
})
.forEach(path => {
if (path.value.callee.type === 'MemberExpression' && path.value.callee.property.type === 'Identifier') {
const comment = methodCommentMap[path.value.callee.property.name];
if (comment) {
addTodoComment(j, path, comment);
}
}
});

// Find `new Hub()` & `new Sentry.Hub()`
tree.find(j.NewExpression).forEach(path => {
if (
(path.value.callee.type === 'Identifier' && path.value.callee.name === 'Hub') ||
(path.value.callee.type === 'MemberExpression' &&
path.value.callee.property.type === 'Identifier' &&
path.value.callee.property.name === 'Hub')
) {
addTodoComment(
j,
path,
'Use `new Scope()` instead - see https://github.com/getsentry/sentry-javascript/blob/develop/docs/v8-initializing.md'
);
}
});

return tree.toSource();
});
};

/**
*
* @param {import('jscodeshift')} j
* @param {import('jscodeshift').ASTPath} path
* @param {string} msg
*/
function addTodoComment(j, path, msg) {
const commentPath = getCommentPath(path);

const type = commentPath.value.type;

if (type !== 'CallExpression' && type !== 'VariableDeclaration' && type !== 'ObjectProperty') {
return;
}

const comments = (commentPath.value.comments = commentPath.value.comments || []);
comments.push(j.commentLine(` TODO(sentry): ${msg}`, true, true));
}

/**
* Get the path where we want to attach the comment too.
* Without this, sometimes the comment would be in a visually unpleasing place.
*
* @param {import('jscodeshift').ASTPath} path
* @returns {import('jscodeshift').ASTPath}
*/
function getCommentPath(path) {
// We try some combinations here that we special case:
// const x = foo();
if (path.parent.parent.value.type === 'VariableDeclaration') {
return path.parent.parent;
}

// { a: foo() }
if (path.parent.value.type === 'ObjectProperty') {
return path.parent;
}

return path;
}
12 changes: 12 additions & 0 deletions test-fixtures/addMigrationComments/hub.js
Copy link
Member

Choose a reason for hiding this comment

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

l: Can we add a test for top-level and function body usage of these APIs? Judinging by this test it should work but it probably makes sense to have this rather likely scenario covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, done!

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from '@sentry/browser';
import { makeMain, Hub } from '@sentry/browser';

function doSomething() {
const hub = new Hub();
makeMain(hub);
}

function doSomethingElse() {
const hub = new Sentry.Hub();
Sentry.makeMain(hub);
}
10 changes: 10 additions & 0 deletions test-fixtures/addMigrationComments/startChild.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

function doSomething(span) {
span.startChild();
}

function doSomethingElse() {
const transaction = Sentry.getActiveSpan();
transaction.startChild().end();
transaction.finish();
}
15 changes: 15 additions & 0 deletions test-fixtures/addMigrationComments/startTransaction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as Sentry from '@sentry/browser';
import { startTransaction } from '@sentry/browser';

function doSomething() {
startTransaction();
}

function doSomethingElse() {
const transaction = Sentry.startTransaction();
transaction.finish();

const obj = {
transaction: Sentry.startTransaction(),
};
}
Loading