-
Notifications
You must be signed in to change notification settings - Fork 470
fix: stop shipping broken source maps and drop direct node-fetch dep #8453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * This script runs at pack time (`prepack`), before the tarball is assembled. | ||
| * | ||
| * We build with `sourceMap` and `declarationMap` enabled so that local development against `dist/` | ||
| * has working maps. Those maps are useless to end users, though: they reference `../src/*.ts`, and | ||
| * `src` is not in the package's `files` list, so every map in a published install points at a file | ||
| * that isn't there. They accounted for roughly half of the published package, so we strip them | ||
| * here rather than shipping dead weight. | ||
| * | ||
| * This is idempotent — `npm publish` is invoked more than once per release (see | ||
| * `.github/workflows/release-please.yml`), and the second run should be a no-op. | ||
| */ | ||
|
|
||
| import { readdir, rm, readFile, stat, writeFile } from 'node:fs/promises' | ||
| import path from 'node:path' | ||
| import { fileURLToPath } from 'node:url' | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)) | ||
| const DIST_DIR = path.join(__dirname, '..', 'dist') | ||
| // `tsc --incremental` decides what to emit from this file alone, not from what's on disk. Stripping | ||
| // maps out from under it would otherwise leave a subsequent local build convinced it has nothing to | ||
| // do, silently yielding a `dist/` with no maps (or, after `npm run clean`, no `dist/` at all). | ||
| const BUILD_INFO_FILE = path.join(__dirname, '..', 'tsconfig.build.tsbuildinfo') | ||
|
|
||
| // Matches the trailing `//# sourceMappingURL=...` annotation left behind once the map is gone. | ||
| const SOURCE_MAPPING_URL_RE = /^\/\/# sourceMappingURL=.*$\n?/gm | ||
|
|
||
| const walk = async (dir) => { | ||
| let entries | ||
| try { | ||
| entries = await readdir(dir, { withFileTypes: true }) | ||
| } catch (error) { | ||
| if (error.code === 'ENOENT') { | ||
| return [] | ||
| } | ||
| throw error | ||
| } | ||
|
|
||
| const files = await Promise.all( | ||
| entries.map(async (entry) => { | ||
| const entryPath = path.join(dir, entry.name) | ||
| return entry.isDirectory() ? walk(entryPath) : [entryPath] | ||
| }), | ||
| ) | ||
| return files.flat() | ||
| } | ||
|
|
||
| const stripSourceMaps = async () => { | ||
| const files = await walk(DIST_DIR) | ||
|
|
||
| if (files.length === 0) { | ||
| console.error('strip-source-maps: no dist/ output found, nothing to do') | ||
| return | ||
|
Comment on lines
+51
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- scripts/strip-source-maps.js ---'
cat -n scripts/strip-source-maps.js
printf '%s\n' '--- package files and build references ---'
rg -n --glob 'package.json' --glob 'tsconfig*.json' --glob '*.md' \
'typescript|tsc|tsbuildinfo|incremental|strip-source-maps|prepack|dist/' .Repository: netlify/cli Length of output: 11823 🌐 Web query:
💡 Result: The issue where TypeScript does not recreate output files in your dist folder after they are deleted occurs because the incremental build system relies on the.tsbuildinfo file to determine if a project is up to date [1][2]. When you delete your output files but leave the.tsbuildinfo file intact, TypeScript assumes the previous build is still valid based on the timestamps stored in that file and skips the emission of new files [2][3]. This behavior is "working as designed" by the TypeScript team to avoid the performance cost of checking the existence of every output file on every build [4][2]. To resolve this and ensure your output files are recreated, you can use one of the following methods: 1. Use the force flag: Run your build command with the --force option (e.g., tsc -b --force). This instructs TypeScript to ignore the incremental status and perform a full rebuild of the project [5][6]. 2. Clean the build artifacts: Run the clean command if you are using build mode (tsc -b --clean). This command is specifically designed to safely remove all build outputs, including the.tsbuildinfo file, ensuring a fresh start for the next build [7][2]. 3. Manually delete the.tsbuildinfo file: If you are manually cleaning your dist directory via scripts, ensure your process also deletes the associated.tsbuildinfo file [1][4]. If the.tsbuildinfo file is missing, TypeScript will treat the build as non-incremental and perform a complete compilation [8][3]. 4. Adjust build scripts: If you are using tools like NestJS or custom CLI scripts that automatically clear output directories, consider updating those scripts to also delete the.tsbuildinfo file to prevent this stale state [9][3]. Citations:
Remove build metadata before returning when This branch returns before 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| const maps = files.filter((file) => file.endsWith('.map')) | ||
| const annotated = files.filter((file) => file.endsWith('.js') || file.endsWith('.d.ts')) | ||
|
|
||
| // Collect sizes first and sum at the end: `total += await size(file)` would read `total` before | ||
| // awaiting and clobber every concurrent update. | ||
| const bytesRemoved = ( | ||
| await Promise.all( | ||
| maps.map(async (file) => { | ||
| const { size } = await stat(file) | ||
| await rm(file) | ||
| return size | ||
| }), | ||
| ) | ||
| ).reduce((total, size) => total + size, 0) | ||
|
|
||
| let annotationsRemoved = 0 | ||
| await Promise.all( | ||
| annotated.map(async (file) => { | ||
| const contents = await readFile(file, 'utf8') | ||
| const stripped = contents.replace(SOURCE_MAPPING_URL_RE, '') | ||
| if (stripped !== contents) { | ||
| annotationsRemoved += 1 | ||
| await writeFile(file, stripped) | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| await rm(BUILD_INFO_FILE, { force: true }) | ||
|
|
||
| console.error( | ||
| `strip-source-maps: removed ${maps.length.toString()} map file(s) (${(bytesRemoved / 1024 / 1024).toFixed( | ||
| 2, | ||
| )} MB) and ${annotationsRemoved.toString()} sourceMappingURL annotation(s)`, | ||
| ) | ||
| } | ||
|
|
||
| await stripSourceMaps() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,5 @@ | ||
| import URL from 'url' | ||
|
|
||
| import fetch from 'node-fetch' | ||
|
|
||
| // supported repo host types | ||
| const GITHUB = 'GitHub' | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove implementation comments that violate the JavaScript guideline.
These comments explain control flow and implementation details. Keep the code self-explanatory, or move non-obvious build-contract rationale into maintained documentation.
As per coding guidelines:
**/*.{js,jsx,ts,tsx,mjs,cjs,go,rs}: Never write comments on what the code does, make the code clean and self explanatory instead.Also applies to: 25-25, 59-60
🤖 Prompt for AI Agents
Source: Coding guidelines