chore: Don't use extrenalNames#2505
Merged
Merged
Conversation
|
pkg.pr.new packages benchmark commit |
aleksanderkatan
changed the base branch from
main
to
impr/better-metadata-versioning
📊 Bundle Size Comparison
👀 Notable resultsStatic test results:No major changes. Dynamic test results:No major changes. 📋 All resultsClick to reveal the results table (353 entries).
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu. |
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.86, 1.64, 3.44, 5.66, 6.20, 9.58, 18.25, 18.19]
line [0.90, 1.78, 3.84, 5.58, 6.64, 8.40, 18.02, 19.88]
line [0.88, 1.83, 3.44, 5.59, 6.69, 8.52, 19.70, 20.73]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.29, 0.50, 0.66, 0.75, 1.05, 1.06, 1.25, 1.42]
line [0.32, 0.52, 0.66, 0.75, 1.08, 1.11, 1.37, 1.49]
line [0.26, 0.49, 0.66, 0.86, 1.14, 1.14, 1.39, 1.52]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.87, 1.65, 2.94, 6.35, 10.75, 22.57, 47.14, 96.62]
line [0.81, 1.71, 3.58, 5.42, 10.60, 22.95, 48.73, 100.28]
line [0.84, 1.88, 3.44, 5.60, 10.79, 22.44, 49.75, 102.64]
|
aleksanderkatan
changed the base branch from
impr/better-metadata-versioning
to
main
There was a problem hiding this comment.
Pull request overview
This PR removes reliance on ast.externalNames in the TypeGPU runtime by dropping the “missing external links” validation and removing the associated MissingLinksError from the public API, while also narrowing the normalized metadata AST type to omit externalNames.
Changes:
- Remove
MissingLinksError(class + named export) and the corresponding runtime validation infnCore. - Update
Metadata.astto no longer exposeexternalNamesafter normalization. - Add a TODO in the unplugin metadata factory noting planned removal of
externalNamesfrom the AST.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/unplugin-typegpu/src/core/factory.ts | Adds TODO about eventually removing externalNames from the emitted AST. |
| packages/typegpu/src/shared/normalizeMetadata.ts | Narrows normalized Metadata.ast type to exclude externalNames. |
| packages/typegpu/src/indexNamedExports.ts | Removes MissingLinksError from public named exports (API surface). |
| packages/typegpu/src/errors.ts | Deletes MissingLinksError implementation. |
| packages/typegpu/src/core/function/fnCore.ts | Removes MissingLinksError import and the missing-externals validation block. |
Comments suppressed due to low confidence (3)
packages/typegpu/src/indexNamedExports.ts:13
MissingLinksErroris being removed from the public named exports. Sincepackages/typegpu/src/index.jsre-exports everything from this barrel, this is a breaking API change for downstream consumers importing the error type. Consider keeping the export as a deprecated alias for at least one release (or documenting it explicitly and bumping the appropriate semver major).
export {
MissingBindGroupsError,
MissingSlotValueError,
MissingVertexBuffersError,
NotUniformError,
ResolutionError,
} from './errors.ts';
packages/typegpu/src/core/function/fnCore.ts:195
- Removing
MissingLinksErroreliminates a more descriptive error for the "metadata externals don't match used identifiers" case; this now likely degrades to a generic "Identifier X not found" during WGSL generation. If hand-authored/legacy metadata is still a supported path (tests inject raw metadata directly), consider keeping a targeted error (even if only in DEV/TEST) or replacing it with an equivalent diagnostic that includes the function label and missing external names/keys.
// get data generated by the plugin
const pluginData = getFunctionMetadata(implementation);
const pluginExternals = pluginData?.externals;
if (pluginExternals) {
const missing = Object.fromEntries(
Object.entries(pluginExternals).filter(([name]) => !(name in externalMap)),
);
applyExternals(externalMap, missing);
}
const ast = pluginData?.ast;
if (!ast) {
throw new Error(
"Missing metadata for tgpu.fn function body (either missing 'use gpu' directive, or misconfigured `unplugin-typegpu`)",
);
}
// If an entrypoint implementation has a second argument, it represents the output schema.
// We look at the identifier chosen by the user and add it to externals.
const maybeSecondArg = ast.params[1];
if (maybeSecondArg && maybeSecondArg.type === 'i' && functionType !== 'normal') {
applyExternals(externalMap, {
// oxlint-disable-next-line typescript/no-non-null-assertion -- entry functions cannot be shellless
[maybeSecondArg.name]: undecorate(returnType!),
});
}
packages/typegpu/src/errors.ts:155
MissingLinksErroris removed from the library; if this error was part of the public API (it was exported viaindexNamedExports.tspreviously), this is a breaking change. Consider leaving the class (possibly deprecated) for compatibility, or ensure the release notes/semver bump clearly communicate its removal.
export class NotUniformError extends Error {
constructor(value: TgpuBuffer<BaseData>) {
super(
`Buffer '${
getName(value) ?? '<unnamed>'
}' is not bindable as a uniform. Use .$usage('uniform') to allow it.`,
);
// Set the prototype explicitly.
Object.setPrototypeOf(this, NotUniformError.prototype);
}
}
export class MissingBindGroupsError extends Error {
constructor(layouts: Iterable<TgpuBindGroupLayout>) {
super(
`Missing bind groups for layouts: '${[...layouts]
.map((layout) => getName(layout) ?? '<unnamed>')
.join(', ')}'. Please provide it using pipeline.with(bindGroup).(...)`,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blocked by #2502
externalNamesare redundant, and this check infnCorewill pass for every metadata created by unplugin.