Skip to content

Commit 883978b

Browse files
kraenhansenkraenhansen
committed
Refactor naming strategy
1 parent b8b6b92 commit 883978b

File tree

7 files changed

+297
-204
lines changed

7 files changed

+297
-204
lines changed

‎packages/host/react-native-node-api.podspec

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,7 @@ require_relative "./scripts/patch-hermes"
66

77
NODE_PATH ||= `which node`.strip
88
CLI_COMMAND ||= "'#{NODE_PATH}' '#{File.join(__dir__, "dist/node/cli/run.js")}'"
9-
STRIP_PATH_SUFFIX ||= ENV['NODE_API_MODULES_STRIP_PATH_SUFFIX'] === "true"
10-
COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}' #{STRIP_PATH_SUFFIX ? '--strip-path-suffix' : ''}"
9+
COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}'"
1110

1211
# We need to run this now to ensure the xcframeworks are copied vendored_frameworks are considered
1312
XCFRAMEWORKS_DIR ||= File.join(__dir__, "xcframeworks")

‎packages/host/src/node/babel-plugin/plugin.test.ts

Lines changed: 85 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { setupTempDirectory } from "../test-utils.js";
1010
type TestTransformationOptions = {
1111
files: Record<string, string>;
1212
inputFilePath: string;
13-
assertion: (code: string) => void;
13+
assertion(code: string): void;
1414
options?: PluginOptions;
1515
};
1616

@@ -27,10 +27,26 @@ function itTransforms(
2727
assert(result, "Expected transformation to produce a result");
2828
const { code } = result;
2929
assert(code, "Expected transformation to produce code");
30-
assert(assertion(code), `Unexpected code: ${code}`);
30+
assertion(code);
3131
});
3232
}
3333

34+
function assertIncludes(needle: string, { reverse = false } = {}) {
35+
return (code: string) => {
36+
if (reverse) {
37+
assert(
38+
!code.includes(needle),
39+
`Expected code to not include: ${needle}, got ${code}`
40+
);
41+
} else {
42+
assert(
43+
code.includes(needle),
44+
`Expected code to include: ${needle}, got ${code}`
45+
);
46+
}
47+
};
48+
}
49+
3450
describe("plugin", () => {
3551
describe("transforming require(...) calls", () => {
3652
itTransforms("a simple call", {
@@ -44,8 +60,7 @@ describe("plugin", () => {
4460
`,
4561
},
4662
inputFilePath: "index.js",
47-
assertion: (code) =>
48-
code.includes(`requireNodeAddon("my-package--my-addon")`),
63+
assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`),
4964
});
5065

5166
itTransforms("from sub-directory", {
@@ -59,42 +74,57 @@ describe("plugin", () => {
5974
`,
6075
},
6176
inputFilePath: "sub-dir/index.js",
62-
assertion: (code) =>
63-
code.includes(`requireNodeAddon("my-package--my-addon")`),
77+
assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`),
6478
});
6579

66-
itTransforms("addon in sub-directory", {
67-
files: {
68-
"package.json": `{ "name": "my-package" }`,
69-
"sub-dir/my-addon.apple.node/my-addon.node":
70-
"// This is supposed to be a binary file",
71-
"index.js": `
72-
const addon = require('./sub-dir/my-addon.node');
73-
console.log(addon);
74-
`,
75-
},
76-
inputFilePath: "index.js",
77-
assertion: (code) =>
78-
code.includes(`requireNodeAddon("my-package--sub-dir-my-addon")`),
79-
});
80+
describe("in 'sub-dir'", () => {
81+
itTransforms("a nested addon (keeping suffix)", {
82+
files: {
83+
"package.json": `{ "name": "my-package" }`,
84+
"sub-dir/my-addon.apple.node/my-addon.node":
85+
"// This is supposed to be a binary file",
86+
"index.js": `
87+
const addon = require('./sub-dir/my-addon.node');
88+
console.log(addon);
89+
`,
90+
},
91+
inputFilePath: "index.js",
92+
options: { pathSuffix: "keep" },
93+
assertion: assertIncludes(
94+
`requireNodeAddon("my-package--sub-dir-my-addon")`
95+
),
96+
});
8097

81-
itTransforms(
82-
"and returns package name when passed stripPathSuffix option",
83-
{
98+
itTransforms("a nested addon (stripping suffix)", {
8499
files: {
85100
"package.json": `{ "name": "my-package" }`,
86101
"sub-dir/my-addon.apple.node/my-addon.node":
87102
"// This is supposed to be a binary file",
88103
"index.js": `
89-
const addon = require('./sub-dir/my-addon.node');
90-
console.log(addon);
91-
`,
104+
const addon = require('./sub-dir/my-addon.node');
105+
console.log(addon);
106+
`,
92107
},
93108
inputFilePath: "index.js",
94-
options: { stripPathSuffix: true },
95-
assertion: (code) => code.includes(`requireNodeAddon("my-package")`),
96-
}
97-
);
109+
options: { pathSuffix: "strip" },
110+
assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`),
111+
});
112+
113+
itTransforms("a nested addon (omitting suffix)", {
114+
files: {
115+
"package.json": `{ "name": "my-package" }`,
116+
"sub-dir/my-addon.apple.node/my-addon.node":
117+
"// This is supposed to be a binary file",
118+
"index.js": `
119+
const addon = require('./sub-dir/my-addon.node');
120+
console.log(addon);
121+
`,
122+
},
123+
inputFilePath: "index.js",
124+
options: { pathSuffix: "omit" },
125+
assertion: assertIncludes(`requireNodeAddon("my-package")`),
126+
});
127+
});
98128

99129
itTransforms("and does not touch required JS files", {
100130
files: {
@@ -107,8 +137,7 @@ describe("plugin", () => {
107137
`,
108138
},
109139
inputFilePath: "index.js",
110-
options: { stripPathSuffix: true },
111-
assertion: (code) => !code.includes("requireNodeAddon"),
140+
assertion: assertIncludes("requireNodeAddon", { reverse: true }),
112141
});
113142
});
114143

@@ -124,12 +153,28 @@ describe("plugin", () => {
124153
`,
125154
},
126155
inputFilePath: "index.js",
127-
assertion: (code) =>
128-
code.includes(`requireNodeAddon("my-package--my-addon")`),
156+
assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`),
129157
});
130158

131159
describe("in 'build/Release'", () => {
132-
itTransforms("a nested addon", {
160+
itTransforms("a nested addon (keeping suffix)", {
161+
files: {
162+
"package.json": `{ "name": "my-package" }`,
163+
"build/Release/my-addon.apple.node/my-addon.node":
164+
"// This is supposed to be a binary file",
165+
"index.js": `
166+
const addon = require('bindings')('my-addon');
167+
console.log(addon);
168+
`,
169+
},
170+
inputFilePath: "index.js",
171+
options: { pathSuffix: "keep" },
172+
assertion: assertIncludes(
173+
`requireNodeAddon("my-package--build-Release-my-addon")`
174+
),
175+
});
176+
177+
itTransforms("a nested addon (stripping suffix)", {
133178
files: {
134179
"package.json": `{ "name": "my-package" }`,
135180
"build/Release/my-addon.apple.node/my-addon.node":
@@ -140,13 +185,11 @@ describe("plugin", () => {
140185
`,
141186
},
142187
inputFilePath: "index.js",
143-
assertion: (code) =>
144-
code.includes(
145-
`requireNodeAddon("my-package--build-Release-my-addon")`
146-
),
188+
options: { pathSuffix: "strip" },
189+
assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`),
147190
});
148191

149-
itTransforms("strips path suffix when passing stripPathSuffix option", {
192+
itTransforms("a nested addon (omitting suffix)", {
150193
files: {
151194
"package.json": `{ "name": "my-package" }`,
152195
"build/Release/my-addon.apple.node/my-addon.node":
@@ -157,8 +200,8 @@ describe("plugin", () => {
157200
`,
158201
},
159202
inputFilePath: "index.js",
160-
options: { stripPathSuffix: true },
161-
assertion: (code) => code.includes(`requireNodeAddon("my-package")`),
203+
options: { pathSuffix: "omit" },
204+
assertion: assertIncludes(`requireNodeAddon("my-package")`),
162205
});
163206
});
164207
});

‎packages/host/src/node/babel-plugin/plugin.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,27 @@ import {
99
isNodeApiModule,
1010
findNodeAddonForBindings,
1111
NamingStrategy,
12+
PathSuffixChoice,
13+
assertPathSuffix,
1214
} from "../path-utils";
1315

1416
export type PluginOptions = {
15-
stripPathSuffix?: boolean;
17+
/**
18+
* Controls how the path of the addon inside a package is transformed into a library name.
19+
* The transformation is needed to disambiguate and avoid conflicts between addons with the same name (but in different sub-paths or packages).
20+
*
21+
* As an example, if the package name is `my-pkg` and the path of the addon within the package is `build/Release/my-addon.node`:
22+
* - `"omit"`: Only the package name is used and the library name will be `my-pkg`.
23+
* - `"strip"` (default): Path gets stripped to its basename and the library name will be `my-pkg--my-addon`.
24+
* - `"keep"`: The full path is kept and the library name will be `my-pkg--build-Release-my-addon`.
25+
*/
26+
pathSuffix?: PathSuffixChoice;
1627
};
1728

1829
function assertOptions(opts: unknown): asserts opts is PluginOptions {
1930
assert(typeof opts === "object" && opts !== null, "Expected an object");
20-
if ("stripPathSuffix" in opts) {
21-
assert(
22-
typeof opts.stripPathSuffix === "boolean",
23-
"Expected 'stripPathSuffix' to be a boolean"
24-
);
31+
if ("pathSuffix" in opts) {
32+
assertPathSuffix(opts.pathSuffix);
2533
}
2634
}
2735

@@ -49,7 +57,7 @@ export function plugin(): PluginObj {
4957
visitor: {
5058
CallExpression(p) {
5159
assertOptions(this.opts);
52-
const { stripPathSuffix = false } = this.opts;
60+
const { pathSuffix = "strip" } = this.opts;
5361
if (typeof this.filename !== "string") {
5462
// This transformation only works when the filename is known
5563
return;
@@ -72,7 +80,7 @@ export function plugin(): PluginObj {
7280
const resolvedPath = findNodeAddonForBindings(id, from);
7381
if (typeof resolvedPath === "string") {
7482
replaceWithRequireNodeAddon(p.parentPath, resolvedPath, {
75-
stripPathSuffix,
83+
pathSuffix,
7684
});
7785
}
7886
}
@@ -81,7 +89,7 @@ export function plugin(): PluginObj {
8189
isNodeApiModule(path.join(from, id))
8290
) {
8391
const relativePath = path.join(from, id);
84-
replaceWithRequireNodeAddon(p, relativePath, { stripPathSuffix });
92+
replaceWithRequireNodeAddon(p, relativePath, { pathSuffix });
8593
}
8694
}
8795
},

‎packages/host/src/node/cli/options.ts

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
1-
import assert from "node:assert/strict";
2-
31
import { Option } from "@commander-js/extra-typings";
42

5-
const { NODE_API_MODULES_STRIP_PATH_SUFFIX } = process.env;
6-
assert(
7-
typeof NODE_API_MODULES_STRIP_PATH_SUFFIX === "undefined" ||
8-
NODE_API_MODULES_STRIP_PATH_SUFFIX === "true" ||
9-
NODE_API_MODULES_STRIP_PATH_SUFFIX === "false",
10-
"Expected NODE_API_MODULES_STRIP_PATH_SUFFIX to be either 'true' or 'false'"
11-
);
3+
import { assertPathSuffix, PATH_SUFFIX_CHOICES } from "../path-utils";
4+
5+
const { NODE_API_PATH_SUFFIX } = process.env;
6+
if (typeof NODE_API_PATH_SUFFIX === "string") {
7+
assertPathSuffix(NODE_API_PATH_SUFFIX);
8+
}
129

13-
export const stripPathSuffixOption = new Option(
14-
"--strip-path-suffix",
15-
"Don't append escaped relative path to the library names (entails one Node-API module per package)"
16-
).default(NODE_API_MODULES_STRIP_PATH_SUFFIX === "true");
10+
export const pathSuffixOption = new Option(
11+
"--path-suffix <strategy>",
12+
"Controls how the path of the addon inside a package is transformed into a library name"
13+
)
14+
.choices(PATH_SUFFIX_CHOICES)
15+
.default(NODE_API_PATH_SUFFIX || "strip");

0 commit comments

Comments
 (0)