lint(types): integrate arethetypeswrong for local and CI verification#9098
lint(types): integrate arethetypeswrong for local and CI verification#9098mikehardy wants to merge 2 commits into
Conversation
Add plugin/src/app.plugin.ts (export =) compiled to plugin/build/ for Expo entry types and runtime. Keep a one-line package-root app.plugin.js shim for Expo filesystem resolution; wire exports for ./app.plugin and ./app.plugin.js.
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust type-checking gate using attw to ensure published package types align with the project's consumer matrix. It also standardizes Expo config plugin structures, ensuring they are correctly exported and smoke-tested during the CI process to prevent regressions in library consumption. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validation script to check published package types using @arethetypeswrong/cli (attw) and smoke-tests Expo config plugins, alongside updating package exports and documentation. The reviewer feedback is highly actionable and identifies several key improvements for the validation script, including refactoring shell executions from execSync to spawnSync to prevent OS-specific parsing issues, updating yarn pack commands to use Yarn v4 compatible flags (--out instead of --filename), removing a non-existent @expo/config-plugins version specifier that would cause CI failures, and filtering out private packages from the check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'attw-')); | ||
| const tgz = path.join(tmpDir, `${dirName}.tgz`); | ||
| const jsonFile = path.join(tmpDir, `${dirName}.json`); | ||
|
|
||
| const pack = spawnSync('yarn', ['pack', '--filename', tgz], { | ||
| cwd: packageDir, | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }); | ||
| if (pack.status !== 0) { | ||
| throw new Error(`yarn pack failed for ${dirName}: ${pack.stderr || pack.stdout}`); | ||
| } | ||
|
|
||
| let exitCode = 0; | ||
| try { | ||
| execSync( | ||
| `"${attwBin}" --config-path "${attwConfigPath}" --format json --no-color --no-emoji "${tgz}" > "${jsonFile}"`, | ||
| { cwd: tmpDir, stdio: ['ignore', 'pipe', 'pipe'] }, | ||
| ); | ||
| } catch (error) { | ||
| const status = | ||
| typeof error === 'object' && error !== null && 'status' in error | ||
| ? Number((error as { status?: number }).status) | ||
| : 1; | ||
| exitCode = Number.isFinite(status) ? status : 1; | ||
| } | ||
|
|
||
| if (!fs.existsSync(jsonFile) || !fs.readFileSync(jsonFile, 'utf8').trim()) { | ||
| throw new Error(`attw produced no output for ${dirName}`); | ||
| } | ||
|
|
||
| const data = JSON.parse(fs.readFileSync(jsonFile, 'utf8')) as AttwResult; |
There was a problem hiding this comment.
Refactor runAttw to use spawnSync with arguments instead of execSync with manual string interpolation and shell redirection. This avoids shell execution overhead, prevents potential command parsing issues across different OS platforms (like Windows), and eliminates the need to write and read a temporary JSON file. Additionally, this fixes the yarn pack option from --filename (Yarn v1) to --out (Yarn v4).
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'attw-'));
const tgz = path.join(tmpDir, `${dirName}.tgz`);
const pack = spawnSync('yarn', ['pack', '--out', tgz], {
cwd: packageDir,
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'],
});
if (pack.status !== 0) {
throw new Error(`yarn pack failed for ${dirName}: ${pack.stderr || pack.stdout}`);
}
const result = spawnSync(
attwBin,
['--config-path', attwConfigPath, '--format', 'json', '--no-color', '--no-emoji', tgz],
{ cwd: tmpDir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], shell: true }
);
const exitCode = result.status ?? 1;
const stdout = result.stdout?.trim();
try {
fs.unlinkSync(tgz);
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// ignore cleanup errors
}
if (!stdout) {
throw new Error(`attw produced no output for ${dirName}: ${result.stderr || ''}`);
}
const data = JSON.parse(stdout) as AttwResult;There was a problem hiding this comment.
Resolved in b60d54172 (amended into lint(types)).
Accepted: runAttw now uses spawnSync with argument arrays for both yarn pack and attw — no shell, no string interpolation.
Partially declined (temp JSON file): We kept file-based capture rather than parsing stdout directly. spawnSync now writes attw output straight to a file descriptor (stdio: ['ignore', outFd, 'pipe']), which avoids shell redirection and guarantees the full JSON report is captured regardless of size (no maxBuffer truncation). Truncation safety is more important than eliminating the temp file here.
Also applied: yarn pack --out (Yarn v4 flag).
| } | ||
|
|
||
| function packPackage(packageDir: string, outputPath: string): void { | ||
| const pack = spawnSync('yarn', ['pack', '--filename', outputPath], { |
There was a problem hiding this comment.
In Yarn v4 (Berry), the yarn pack command uses --out or -o to specify the output filename instead of --filename (which was used in Yarn v1). Using --filename will result in a CLI usage error in Yarn v4.
| const pack = spawnSync('yarn', ['pack', '--filename', outputPath], { | |
| const pack = spawnSync('yarn', ['pack', '--out', outputPath], { |
There was a problem hiding this comment.
Resolved in b60d54172 (amended into lint(types)).
Accepted: packPackage now uses yarn pack --out (Yarn v4) instead of --filename.
| execSync('npm init -y', { cwd: installDir, stdio: 'ignore' }); | ||
|
|
||
| const installArgs = [ | ||
| 'install', | ||
| '--silent', | ||
| '--no-fund', | ||
| '--no-audit', | ||
| appTgz, | ||
| pkgTgz, | ||
| '@expo/config-plugins@54', | ||
| ]; | ||
| try { | ||
| execSync(`npm ${installArgs.join(' ')}`, { | ||
| cwd: installDir, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| failures.push({ | ||
| package: JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8')).name, | ||
| detail: `npm install failed: ${message}`, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| const pkgName = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8')).name as string; | ||
| try { | ||
| execSync( | ||
| `node -e "const plugin = require('${pkgName}/app.plugin.js'); if (typeof plugin !== 'function' && typeof plugin.default !== 'function') { process.exit(2); }"`, | ||
| { cwd: installDir, stdio: ['ignore', 'pipe', 'pipe'] }, | ||
| ); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| failures.push({ | ||
| package: pkgName, | ||
| detail: `require('${pkgName}/app.plugin.js') failed: ${message}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Refactor checkExpoPlugins to use spawnSync instead of execSync to avoid shell execution and potential command parsing issues. Also, remove the hardcoded @expo/config-plugins@54 version specifier, as version 54 does not exist on npm and will cause the installation to fail in CI. Installing @expo/config-plugins without a version specifier or with a valid range is safer and more robust.
spawnSync('npm', ['init', '-y'], { cwd: installDir, stdio: 'ignore', shell: true });
const installArgs = [
'install',
'--silent',
'--no-fund',
'--no-audit',
appTgz,
pkgTgz,
'@expo/config-plugins',
];
const installResult = spawnSync('npm', installArgs, {
cwd: installDir,
stdio: ['ignore', 'pipe', 'pipe'],
shell: true,
});
if (installResult.status !== 0) {
failures.push({
package: JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8')).name,
detail: `npm install failed: ${installResult.stderr?.toString() || 'unknown error'}`,
});
continue;
}
const pkgName = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8')).name as string;
const runResult = spawnSync(
'node',
[
'-e',
`const plugin = require('${pkgName}/app.plugin.js'); if (typeof plugin !== 'function' && typeof plugin.default !== 'function') { process.exit(2); }`,
],
{ cwd: installDir, stdio: ['ignore', 'pipe', 'pipe'] }
);
if (runResult.status !== 0) {
failures.push({
package: pkgName,
detail: `require('${pkgName}/app.plugin.js') failed: ${runResult.stderr?.toString() || 'unknown error'}`,
});
}There was a problem hiding this comment.
Resolved in b60d54172 (amended into lint(types)).
Accepted: checkExpoPlugins refactored to spawnSync for npm init, npm install, and the node -e require check — no shell execution.
Accepted (unpinned): @expo/config-plugins@54 removed; smoke test now installs @expo/config-plugins unpinned so we track the latest release. Expo consumers upgrade quickly, and this acts as an early-warning canary for breaking config-plugin API changes. Verified locally: smoke passes against current latest (57.x).
Note: @expo/config-plugins@54 does exist on npm (54.0.0–54.0.4) and CI was already green with the pin — the unpinned change is intentional drift detection, not a fix for a missing version.
| function listPublishedPackages(): string[] { | ||
| return fs | ||
| .readdirSync(packagesDir) | ||
| .filter(dir => fs.existsSync(path.join(packagesDir, dir, 'package.json'))) | ||
| .sort(); | ||
| } |
There was a problem hiding this comment.
Filter out private packages from the list of published packages. Private packages (marked with "private": true in package.json) are not published to npm, so running attw on them is unnecessary and may fail if they are not configured for publishing.
function listPublishedPackages(): string[] {
return fs
.readdirSync(packagesDir)
.filter(dir => {
const pkgJsonPath = path.join(packagesDir, dir, 'package.json');
if (!fs.existsSync(pkgJsonPath)) return false;
try {
const pkg = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8'));
return pkg.private !== true;
} catch {
return false;
}
})
.sort();
}There was a problem hiding this comment.
Resolved in b60d54172 (amended into lint(types)).
Accepted: listPublishedPackages() now skips packages with "private": true in package.json. None of the current packages/* are private, but this is cheap future-proofing.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #9098 +/- ##
============================================
- Coverage 58.66% 53.13% -5.52%
+ Complexity 1617 1616 -1
============================================
Files 503 428 -75
Lines 39180 33646 -5534
Branches 5807 4636 -1171
============================================
- Hits 22981 17875 -5106
+ Misses 14800 14320 -480
- Partials 1399 1451 +52
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Add yarn attw:check (scoped via .attw.json esm-only profile, ignoring node16-esm internal-resolution and cjs-resolves-to-esm per Types-AD-2) plus Expo plugin consumer smoke. Document consumer-matrix decisions in OKF architecture-decisions.md (Types-AD-1..4).
mikehardy
force-pushed
the
arethetypeswrong
branch
from
4712cf7 to
b60d541
Compare
|
@gemini-code-assist — all review feedback addressed in
Verified: |
|
Thanks for the update, Mike. The changes look solid. Your reasoning for keeping the file-descriptor capture for the |
1 participant
Description
Adds scoped published-types verification with
@arethetypeswrong/clifor local use (yarn attw:check) and CI (newattwjob inlinting.yml).Scope follows our consumer matrix (Types-AD-1..4 in
okf-bundle/testing/architecture-decisions.md):yarn tsc:compile:consumer(moduleResolution: bundler)..attw.json(esm-onlyprofile; ignoreinternal-resolution-errorandcjs-resolves-to-esm) — Node10/Node16-CJS modes and extensionless.d.tsrelative imports are out of scope for RN/Metro consumers.npm pack→ temp install →require('<pkg>/app.plugin.js')), not attw exclusion.Companion commit refactors Expo plugins:
plugin/src/app.plugin.tscompiles toplugin/build/app.plugin.{js,d.ts}; package root keeps a one-lineapp.plugin.jsshim;exportswire./app.pluginand./app.plugin.jstypes to the built entry.Related issues
N/A — proactive types hygiene / CI gate.
Release Summary
No user-facing runtime changes. Published Expo plugin type exports and CI type-checking coverage improved.
Checklist
AndroidiOSOther(macOS, web) — tooling / published package types / Expo config pluginse2etests added or updated inpackages/**/e2ejesttests added or updated inpackages/**/__tests__Test Plan
yarn attw:check— all 19 published packages pass scoped attw; Expo plugin consumer smoke passesyarn lerna run build:plugin --scope '@react-native-firebase/{app,analytics,auth,messaging,crashlytics,perf,app-check,app-distribution}'before attw (plugin/build artifacts)Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter