Skip to content

Commit 6ce9b9d

Browse files
kraenhansenkraenhansen
committed
Refactor naming strategy
1 parent b8b6b92 commit 6ce9b9d

File tree

7 files changed

+291
-197
lines changed

7 files changed

+291
-197
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: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,16 @@ import {
1212
} from "../path-utils";
1313

1414
export type PluginOptions = {
15-
stripPathSuffix?: boolean;
15+
/**
16+
* Controls how the path of the addon inside a package is transformed into a library name.
17+
* The transformation is needed to disambiguate and avoid conflicts between addons with the same name (but in different sub-paths or packages).
18+
*
19+
* 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`:
20+
* - `"omit"`: Only the package name is used and the library name will be `my-pkg`.
21+
* - `"strip"` (default): Path gets stripped to its basename and the library name will be `my-pkg--my-addon`.
22+
* - `"keep"`: The full path is kept and the library name will be `my-pkg--build-Release-my-addon`.
23+
*/
24+
pathSuffix?: "strip" | "omit" | "keep";
1625
};
1726

1827
function assertOptions(opts: unknown): asserts opts is PluginOptions {
@@ -49,7 +58,7 @@ export function plugin(): PluginObj {
4958
visitor: {
5059
CallExpression(p) {
5160
assertOptions(this.opts);
52-
const { stripPathSuffix = false } = this.opts;
61+
const { pathSuffix = "strip" } = this.opts;
5362
if (typeof this.filename !== "string") {
5463
// This transformation only works when the filename is known
5564
return;
@@ -72,7 +81,7 @@ export function plugin(): PluginObj {
7281
const resolvedPath = findNodeAddonForBindings(id, from);
7382
if (typeof resolvedPath === "string") {
7483
replaceWithRequireNodeAddon(p.parentPath, resolvedPath, {
75-
stripPathSuffix,
84+
pathSuffix,
7685
});
7786
}
7887
}
@@ -81,7 +90,7 @@ export function plugin(): PluginObj {
8190
isNodeApiModule(path.join(from, id))
8291
) {
8392
const relativePath = path.join(from, id);
84-
replaceWithRequireNodeAddon(p, relativePath, { stripPathSuffix });
93+
replaceWithRequireNodeAddon(p, relativePath, { pathSuffix });
8594
}
8695
}
8796
},

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

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,31 @@ import assert from "node:assert/strict";
22

33
import { Option } from "@commander-js/extra-typings";
44

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-
);
5+
import { NamingStrategy } from "../path-utils";
126

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");
7+
const PATH_SUFFIX_CHOICES = [
8+
"strip",
9+
"keep",
10+
"omit",
11+
] as const satisfies readonly NamingStrategy["pathSuffix"][];
12+
13+
type PathSuffixChoice = (typeof PATH_SUFFIX_CHOICES)[number];
14+
15+
function assertPathSuffix(value: string): asserts value is PathSuffixChoice {
16+
assert(
17+
(PATH_SUFFIX_CHOICES as readonly string[]).includes(value),
18+
`Expected one of ${PATH_SUFFIX_CHOICES.join(", ")}`
19+
);
20+
}
21+
22+
const { NODE_API_PATH_SUFFIX } = process.env;
23+
if (typeof NODE_API_PATH_SUFFIX === "string") {
24+
assertPathSuffix(NODE_API_PATH_SUFFIX);
25+
}
26+
27+
export const pathSuffixOption = new Option(
28+
"--path-suffix <strategy>",
29+
"Controls how the path of the addon inside a package is transformed into a library name"
30+
)
31+
.choices(PATH_SUFFIX_CHOICES)
32+
.default(NODE_API_PATH_SUFFIX || "strip");

0 commit comments

Comments
 (0)