Skip to content

Commit 3e7242a

Browse files
fix(schematics): address review feedback on the prerelease pin
- warn (instead of silently skipping) when the installed version can't be determined - pin the @angular/fire entry whether it sits in dependencies or devDependencies - resolve the installed version from a build-written placeholder rather than a runtime readFileSync/__dirname traversal; the build fails if the placeholder is not substituted, so a broken pin can't ship silently
1 parent dda001e commit 3e7242a

3 files changed

Lines changed: 71 additions & 33 deletions

File tree

‎src/schematics/common.jasmine.ts‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,22 @@ import 'jasmine';
55

66
const context = { logger: new logging.Logger('test') } as unknown as SchematicContext;
77

8-
const treeWithAngularFire = (declaredVersion: string) => {
8+
const treeWithAngularFire = (declaredVersion: string, section = 'dependencies') => {
99
const tree = new HostTree();
1010
tree.create('package.json', JSON.stringify({
1111
name: 'test-app',
12-
dependencies: {
12+
[section]: {
1313
'@angular/fire': declaredVersion,
1414
firebase: '^12.4.0',
1515
},
1616
}, null, 2));
1717
return tree;
1818
};
1919

20-
const dependenciesIn = (tree: HostTree) =>
21-
JSON.parse(tree.readText('package.json')).dependencies;
20+
const sectionIn = (tree: HostTree, section: string) =>
21+
JSON.parse(tree.readText('package.json'))[section];
22+
23+
const dependenciesIn = (tree: HostTree) => sectionIn(tree, 'dependencies');
2224

2325
describe('pinInstalledPrereleaseVersion', () => {
2426

@@ -64,12 +66,24 @@ describe('pinInstalledPrereleaseVersion', () => {
6466
expect(dependenciesIn(tree)['@angular/fire']).toBe('^21.0.0-rc.0');
6567
});
6668

67-
it('does not pin when the installed version is not valid semver', () => {
69+
it('warns and does not pin when the installed version is not valid semver', () => {
70+
const warnSpy = spyOn(context.logger, 'warn');
6871
const tree = treeWithAngularFire('^21.0.0-rc.0');
6972
pinInstalledPrereleaseVersion(tree, context, 'ANGULARFIRE2_VERSION');
73+
expect(warnSpy).toHaveBeenCalled();
7074
expect(dependenciesIn(tree)['@angular/fire']).toBe('^21.0.0-rc.0');
7175
});
7276

77+
it('does nothing when the @angular/fire entry is not a string', () => {
78+
const tree = new HostTree();
79+
tree.create('package.json', JSON.stringify({
80+
name: 'test-app',
81+
dependencies: { '@angular/fire': { version: '^21.0.0-rc.0' } },
82+
}));
83+
expect(() => pinInstalledPrereleaseVersion(tree, context, '21.0.0-rc.0')).not.toThrow();
84+
expect(dependenciesIn(tree)['@angular/fire']).toEqual({ version: '^21.0.0-rc.0' });
85+
});
86+
7387
it('does nothing when @angular/fire is not a dependency', () => {
7488
const tree = new HostTree();
7589
tree.create('package.json', JSON.stringify({ name: 'test-app', dependencies: {} }));
@@ -82,10 +96,22 @@ describe('pinInstalledPrereleaseVersion', () => {
8296
expect(() => pinInstalledPrereleaseVersion(tree, context, '21.0.0-rc.0')).not.toThrow();
8397
});
8498

85-
it('skips gracefully when the installed version cannot be determined', () => {
86-
const tree = treeWithAngularFire('^21.0.0-rc.0');
87-
pinInstalledPrereleaseVersion(tree, context, undefined);
88-
expect(dependenciesIn(tree)['@angular/fire']).toBe('^21.0.0-rc.0');
99+
it('pins a prerelease range found in devDependencies', () => {
100+
const tree = treeWithAngularFire('^21.0.0-rc.0', 'devDependencies');
101+
pinInstalledPrereleaseVersion(tree, context, '21.0.0-rc.0');
102+
expect(sectionIn(tree, 'devDependencies')['@angular/fire']).toBe('21.0.0-rc.0');
103+
});
104+
105+
it('pins the dependencies entry when @angular/fire appears in both sections', () => {
106+
const tree = new HostTree();
107+
tree.create('package.json', JSON.stringify({
108+
name: 'test-app',
109+
dependencies: { '@angular/fire': '^21.0.0-rc.0' },
110+
devDependencies: { '@angular/fire': '^21.0.0-rc.0' },
111+
}, null, 2));
112+
pinInstalledPrereleaseVersion(tree, context, '21.0.0-rc.0');
113+
expect(dependenciesIn(tree)['@angular/fire']).toBe('21.0.0-rc.0');
114+
expect(sectionIn(tree, 'devDependencies')['@angular/fire']).toBe('^21.0.0-rc.0');
89115
});
90116

91117
});

‎src/schematics/common.ts‎

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import { readFileSync } from 'fs';
2-
import { join } from 'path';
31
import { SchematicContext, SchematicsException, Tree } from '@angular-devkit/schematics';
42
import {
53
intersects as semverIntersects,
@@ -73,19 +71,9 @@ export const addDependencies = (
7371
overwriteIfExists(host, 'package.json', stringifyFormatted(packageJson));
7472
};
7573

76-
/**
77-
* Reads the exact version of the installed `@angular/fire` package from its own manifest — the
78-
* compiled schematics run from `<package root>/schematics/<entry>/`, so the manifest sits two
79-
* directories up. Returns undefined when that layout doesn't hold (e.g. running from source),
80-
* so callers skip pinning instead of failing the schematic.
81-
*/
82-
const readInstalledVersion = () => {
83-
try {
84-
return JSON.parse(readFileSync(join(__dirname, '..', '..', 'package.json')).toString()).version;
85-
} catch {
86-
return undefined;
87-
}
88-
};
74+
// The build writes the published version over this placeholder in the compiled schematics
75+
// (tools/build.ts, replaceSchematicVersions); running from source leaves the placeholder.
76+
const angularFireVersion = 'ANGULARFIRE2_VERSION';
8977

9078
/**
9179
* Pins the workspace's `@angular/fire` entry to the exact installed version when `ng add` wrote a
@@ -96,23 +84,31 @@ const readInstalledVersion = () => {
9684
export const pinInstalledPrereleaseVersion = (
9785
host: Tree,
9886
context: SchematicContext,
99-
installedVersion = readInstalledVersion(),
87+
installedVersion = angularFireVersion,
10088
) => {
10189
if (!host.exists('package.json')) { return; }
10290
const packageJson = safeReadJSON('package.json', host);
10391

104-
const declaredAngularFireVersion = packageJson.dependencies?.['@angular/fire'];
105-
if (
106-
typeof declaredAngularFireVersion !== 'string' ||
107-
!(declaredAngularFireVersion.startsWith('^') || declaredAngularFireVersion.startsWith('~'))
108-
) { return; }
92+
const dependencySection = ['dependencies', 'devDependencies'].find(
93+
section => typeof packageJson[section]?.['@angular/fire'] === 'string',
94+
);
95+
if (!dependencySection) { return; }
96+
97+
const declaredAngularFireVersion = packageJson[dependencySection]['@angular/fire'];
98+
if (!(declaredAngularFireVersion.startsWith('^') || declaredAngularFireVersion.startsWith('~'))) { return; }
99+
100+
if (!semverValid(installedVersion)) {
101+
context.logger.warn(
102+
'Could not determine the installed @angular/fire version; leaving the declared version range as-is.'
103+
);
104+
return;
105+
}
109106

110107
if (
111-
semverValid(installedVersion) &&
112108
semverPrerelease(installedVersion) &&
113109
semverSatisfies(installedVersion, declaredAngularFireVersion, { includePrerelease: true })
114110
) {
115-
packageJson.dependencies['@angular/fire'] = installedVersion;
111+
packageJson[dependencySection]['@angular/fire'] = installedVersion;
116112
overwriteIfExists(host, 'package.json', stringifyFormatted(packageJson));
117113
context.logger.info(
118114
`Pinned @angular/fire to the exact version ${installedVersion} — a prerelease range like ` +

‎tools/build.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { spawn } from 'cross-spawn';
22
import { copy, writeFile } from 'fs-extra';
3-
import { join } from 'path';
3+
import { join, sep } from 'path';
44
import { keys as tsKeys } from 'ts-transformer-keys';
55
import * as esbuild from "esbuild";
66

@@ -273,6 +273,21 @@ async function replacePackageCoreVersion() {
273273
});
274274
}
275275

276+
async function writeVersionOverPlaceholder(root: { version: string }) {
277+
const replace = require('replace-in-file');
278+
const replacements = await replace({
279+
// Glob patterns need forward slashes even on Windows.
280+
files: `${dest('schematics').split(sep).join('/')}/**/*.js`,
281+
from: /ANGULARFIRE2_VERSION/g,
282+
to: root.version,
283+
// Zero matches should reach the guard below, not replace-in-file's generic rejection.
284+
allowEmptyPaths: true,
285+
});
286+
if (!replacements.some(replacement => replacement.hasChanged)) {
287+
throw new Error('No ANGULARFIRE2_VERSION placeholder found in the compiled schematics — the version pin would ship disabled.');
288+
}
289+
}
290+
276291
async function replaceSchematicVersions() {
277292
const root = await rootPackage;
278293
const packagesPath = dest('schematics', 'versions.json');
@@ -283,6 +298,7 @@ async function replaceSchematicVersions() {
283298
Object.keys(dependencies.firebaseFunctionsDependencies).forEach(name => {
284299
dependencies.firebaseFunctionsDependencies[name].version = root.dependencies[name] || root.devDependencies[name];
285300
});
301+
await writeVersionOverPlaceholder(root);
286302
return writeFile(packagesPath, JSON.stringify(dependencies, null, 2));
287303
}
288304

0 commit comments

Comments
 (0)