Skip to content

Commit

Permalink
Remove location property in worklets for production builds (#4695)
Browse files Browse the repository at this point in the history
## Summary

Currently we inject `location` property in worklets - which is there for
debugging purposes - even in production build. This PR corrects this.

Fixes: #4689

## Test plan

Added a new test suite in plugin. Also ran production android Example
just to be sure.
  • Loading branch information
tjzel authored Jul 11, 2023
1 parent 96417d7 commit 3898414
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 20 deletions.
33 changes: 33 additions & 0 deletions __tests__/__snapshots__/plugin.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,24 @@ var f = function () {
}();"
`;

exports[`babel plugin does inject location for worklets in dev builds 1`] = `
"var _worklet_1852213185147_init_data = {
code: "function anonymous(){const x=1;}",
location: "/dev/null"
};
var foo = useAnimatedStyle(function () {
var _e = [new global.Error(), 1, -27];
var _f = function _f() {
var x = 1;
};
_f._closure = {};
_f.__initData = _worklet_1852213185147_init_data;
_f.__workletHash = 1852213185147;
_f.__stackDetails = _e;
return _f;
}());"
`;

exports[`babel plugin doesn't bother other Directive Literals 1`] = `
"function foo() {
'foobar';
Expand All @@ -53,6 +71,21 @@ var f = function () {
}();"
`;

exports[`babel plugin doesn't inject location for worklets in production builds 1`] = `
"var _worklet_1852213185147_init_data = {
code: "function anonymous(){const x=1;}"
};
var foo = useAnimatedStyle(function () {
var _f = function _f() {
var x = 1;
};
_f._closure = {};
_f.__initData = _worklet_1852213185147_init_data;
_f.__workletHash = 1852213185147;
return _f;
}());"
`;

exports[`babel plugin doesn't nest worklets for other threads 1`] = `
"var _worklet_1678749606628_init_data = {
code: "function foo(x){function bar(x){'worklet';return x+2;}return bar(x)+1;}",
Expand Down
33 changes: 32 additions & 1 deletion __tests__/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import { strict as assert } from 'assert';
import '../plugin/jestUtils';
import { version as packageVersion } from '../package.json';

const MOCK_LOCATION = '/dev/null';

function runPlugin(input: string, opts = {}) {
const transformed = transform(input.replace(/<\/?script[^>]*>/g, ''), {
// Our babel presets require us to specify a filename here
// but it is never used so we put in '/dev/null'
// as a safe fallback.
filename: '/dev/null',
filename: MOCK_LOCATION,
compact: false,
plugins: [plugin],
...opts,
Expand Down Expand Up @@ -1125,4 +1127,33 @@ describe('babel plugin', () => {
</script>`;
expect(resultIsIdempotent(input9)).toBe(true);
});

// location
it('does inject location for worklets in dev builds', () => {
const input = html`<script>
const foo = useAnimatedStyle(() => {
const x = 1;
});
</script>`;

const { code } = runPlugin(input);
expect(code).toHaveLocation(MOCK_LOCATION);
expect(code).toMatchSnapshot();
});

it("doesn't inject location for worklets in production builds", () => {
const input = html`<script>
const foo = useAnimatedStyle(() => {
const x = 1;
});
</script>`;

const current = process.env.BABEL_ENV;
process.env.BABEL_ENV = 'production';
const { code } = runPlugin(input, {});
process.env.BABEL_ENV = current;

expect(code).not.toHaveLocation(MOCK_LOCATION);
expect(code).toMatchSnapshot();
});
});
15 changes: 9 additions & 6 deletions plugin/build/plugin.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions plugin/build/plugin.js.map

Large diffs are not rendered by default.

23 changes: 18 additions & 5 deletions plugin/jestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ declare global {
interface Matchers<R> {
toHaveWorkletData(times?: number): R;
toHaveInlineStyleWarning(times?: number): R;
toHaveLocation(location: string): R;
}
}
}
Expand All @@ -21,7 +22,7 @@ expect.extend({
if (receivedMatchCount === expectedMatchCount) {
return {
message: () =>
`Reanimated: expected code to have worklet data ${expectedMatchCount} times`,
`Reanimated: found worklet data ${expectedMatchCount} times`,
pass: true,
};
}
Expand All @@ -31,9 +32,6 @@ expect.extend({
pass: false,
};
},
});

expect.extend({
toHaveInlineStyleWarning(received: string, expectedMatchCount = 1) {
const receivedMatchCount = received.match(
INLINE_STYLE_WARNING_REGEX
Expand All @@ -42,7 +40,7 @@ expect.extend({
if (receivedMatchCount === expectedMatchCount) {
return {
message: () =>
`Reanimated: expected to have inline style warning ${expectedMatchCount} times`,
`Reanimated: found inline style warning ${expectedMatchCount} times`,
pass: true,
};
}
Expand All @@ -52,4 +50,19 @@ expect.extend({
pass: false,
};
},
toHaveLocation(received: string, expectedLocation: string) {
const expectedString = `location: "${expectedLocation}"`;
const hasLocation = received.includes(expectedLocation);
if (hasLocation) {
return {
message: () => `Reanimated: found location ${expectedString}`,
pass: true,
};
}
return {
message: () =>
`Reanimated: expected to have location ${expectedString}, but it's not present`,
pass: false,
};
},
});
21 changes: 15 additions & 6 deletions plugin/src/makeWorklet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,6 @@ export function makeWorklet(
assert(funString, "'funString' is undefined");
const workletHash = hash(funString);

let location = state.file.opts.filename;
if (state.opts.relativeSourceLocation) {
location = relative(state.cwd, location);
}

let lineOffset = 1;
if (variables.length > 0) {
// When worklet captures some variables, we append closure destructing at
Expand All @@ -158,9 +153,23 @@ export function makeWorklet(

const initDataObjectExpression = objectExpression([
objectProperty(identifier('code'), stringLiteral(funString)),
objectProperty(identifier('location'), stringLiteral(location)),
]);

// When testing with jest I noticed that environment variables are set later
// than some functions are evaluated. E.g. this cannot be above this function
// because it would always evaluate to true.
const shouldInjectLocation = !isRelease();
if (shouldInjectLocation) {
let location = state.file.opts.filename;
if (state.opts.relativeSourceLocation) {
location = relative(state.cwd, location);
}

initDataObjectExpression.properties.push(
objectProperty(identifier('location'), stringLiteral(location))
);
}

if (sourceMapString) {
initDataObjectExpression.properties.push(
objectProperty(identifier('sourceMap'), stringLiteral(sourceMapString))
Expand Down

0 comments on commit 3898414

Please sign in to comment.