Skip to content

Commit 81f3f14

Browse files
committed
fix: audit remediation - code quality and documentation improvements
Resolves #178 Code fixes: - builder.ts: Add type guard for error handling (e instanceof Error) - engine.prepare-options-helpers.ts: Use else-if for token injection clarity - ng-add.spec.ts: Replace lazy .toBeTruthy() with proper builder assertion - engine.spec.ts: Replace lazy .toBeDefined() with actual behavior test Documentation fixes: - README.md: Clarify Cloudflare Pages requires --no-notfound for SPA mode - schema.json: Update noNotfound description for Cloudflare Pages - CLAUDE.md: Add 404.html platform behavior, remove fragile line numbers
1 parent a234a2a commit 81f3f14

File tree

7 files changed

+27
-23
lines changed

7 files changed

+27
-23
lines changed

CLAUDE.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ actions.ts (deploy function)
139139
**Note:** `browserTarget` is deliberately HIDDEN from user-facing documentation per deprecation policy, even though it still works internally for backward compatibility.
140140

141141
**Implementation details:**
142-
- Static build target: `browserTarget || buildTarget || default` (see `src/deploy/builder.ts:23-26`)
143-
- Final target: `prerenderTarget || staticBuildTarget` (see `src/deploy/builder.ts:45-50`)
142+
- Static build target: `browserTarget || buildTarget || default` (see `src/deploy/builder.ts`)
143+
- Final target: `prerenderTarget || staticBuildTarget` (see `src/deploy/builder.ts`)
144144

145145
Output directory resolution:
146146
- Checks `angular.json` for `outputPath`
@@ -177,7 +177,10 @@ The engine appends CI metadata to commit messages when running on:
177177

178178
1. **No Server-Side Rendering**: GitHub Pages only supports static files. SSR/Universal build targets are not supported.
179179

180-
2. **404.html Handling**: By default creates `404.html` as copy of `index.html` to handle SPA routing on GitHub Pages. For Cloudflare Pages, disable with `--no-notfound`.
180+
2. **404.html Handling**:
181+
- **GitHub Pages**: Requires `404.html` workaround (only way to get SPA routing, but returns HTTP 404 status)
182+
- **Cloudflare Pages**: MUST NOT have `404.html` - its presence disables native SPA mode
183+
- **Future**: Consider changing default or auto-detecting deployment target
181184

182185
3. **Jekyll Bypass**: Creates `.nojekyll` to prevent GitHub Pages from processing files through Jekyll (which would break files starting with `_` or `.txt` in assets).
183186

README.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -359,9 +359,12 @@ With `--no-dotfiles` files starting with `.` are ignored.
359359
- `ng deploy` – A `404.html` file is created by default.
360360
- `ng deploy --no-notfound` – No `404.html` file is created.
361361

362-
By default, a `404.html` file is created, because this is the only known workaround to avoid 404 error messages on GitHub Pages.
363-
For Cloudflare Pages we highly recommend to disable the `404.html` file by setting this switch to true!
364-
See [#178](https://github.com/angular-schule/angular-cli-ghpages/issues/178)
362+
By default, a `404.html` file is created, because this is the only known workaround for SPA routing on GitHub Pages (note: GitHub still returns HTTP 404 status, which may affect Brave browser and social previews).
363+
364+
> [!IMPORTANT]
365+
> **Cloudflare Pages users:** You **must** use `--no-notfound` to enable native SPA routing. Cloudflare Pages only activates SPA mode when no `404.html` file exists. See [#178](https://github.com/angular-schule/angular-cli-ghpages/issues/178)
366+
>
367+
> We plan to change the default behavior in a future release to better support Cloudflare Pages.
365368
366369
#### --no-nojekyll <a name="no-nojekyll"></a>
367370

src/deploy/builder.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ export default createBuilder(
5050
await deploy(engine, context, finalBuildTarget, options);
5151
} catch (e) {
5252
context.logger.error('❌ An error occurred when trying to deploy:');
53-
context.logger.error(e.message);
53+
const message = e instanceof Error ? e.message : String(e);
54+
context.logger.error(message);
5455
return { success: false };
5556
}
5657

src/deploy/schema.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@
6565
},
6666
"noNotfound": {
6767
"type": "boolean",
68-
"description": "By default a 404.html file is created, because this is the only known workaround to avoid 404 error messages on GitHub Pages. For Cloudflare Pages we highly recommend to disable the 404.html file by setting this switch to true!",
68+
"description": "By default a 404.html file is created for SPA routing on GitHub Pages. For Cloudflare Pages, you must use --no-notfound to enable native SPA routing.",
6969
"default": false
7070
},
7171
"noNojekyll": {

src/engine/engine.prepare-options-helpers.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,22 +173,19 @@ export async function injectTokenIntoRepoUrl(options: PreparedOptions): Promise<
173173
options.repo = options.repo.replace('GH_TOKEN', process.env.GH_TOKEN);
174174
}
175175
// preferred way: token is replaced from plain URL
176+
// Note: Only the first available token is used (GH_TOKEN > PERSONAL_TOKEN > GITHUB_TOKEN)
176177
else if (options.repo && !options.repo.includes('x-access-token:')) {
177178
if (process.env.GH_TOKEN) {
178179
options.repo = options.repo.replace(
179180
'https://github.com/',
180181
`https://x-access-token:${process.env.GH_TOKEN}@github.com/`
181182
);
182-
}
183-
184-
if (process.env.PERSONAL_TOKEN) {
183+
} else if (process.env.PERSONAL_TOKEN) {
185184
options.repo = options.repo.replace(
186185
'https://github.com/',
187186
`https://x-access-token:${process.env.PERSONAL_TOKEN}@github.com/`
188187
);
189-
}
190-
191-
if (process.env.GITHUB_TOKEN) {
188+
} else if (process.env.GITHUB_TOKEN) {
192189
options.repo = options.repo.replace(
193190
'https://github.com/',
194191
`https://x-access-token:${process.env.GITHUB_TOKEN}@github.com/`

src/engine/engine.spec.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,7 @@ describe('engine', () => {
415415
});
416416

417417
it('should monkeypatch util.debuglog before requiring gh-pages', async () => {
418-
// This test verifies the critical ordering requirement documented in engine.ts:22-27
418+
// This test verifies the critical ordering requirement:
419419
// The monkeypatch MUST occur before requiring gh-pages, otherwise gh-pages caches
420420
// the original util.debuglog and our interception won't work.
421421

@@ -429,12 +429,13 @@ describe('engine', () => {
429429
await engine.prepareOptions({}, testLogger);
430430

431431
// Now require gh-pages for the first time (after monkeypatch)
432-
const ghpages = require('gh-pages');
432+
require('gh-pages');
433433

434-
// gh-pages should now use our patched util.debuglog
435-
// We can't directly test gh-pages internal calls, but we verified
436-
// that util.debuglog('gh-pages') forwards to our logger
437-
expect(infoSpy).toBeDefined();
434+
// Verify our patched debuglog('gh-pages') forwards to the logger
435+
const util = require('util');
436+
const ghPagesLogger = util.debuglog('gh-pages');
437+
ghPagesLogger('test message');
438+
expect(infoSpy).toHaveBeenCalledWith('test message');
438439
});
439440
});
440441
});

src/ng-add.spec.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,8 @@ describe('ng-add', () => {
5353
);
5454

5555
const resultConfig = readJSONFromTree(resultTree, 'angular.json');
56-
expect(
57-
resultConfig.projects[OTHER_PROJECT_NAME].architect.deploy
58-
).toBeTruthy();
56+
const deployTarget = resultConfig.projects[OTHER_PROJECT_NAME].architect.deploy;
57+
expect(deployTarget.builder).toBe('angular-cli-ghpages:deploy');
5958
});
6059
});
6160

0 commit comments

Comments
 (0)