fix(hmr): split component ids on the last @ - #492
Open
ashley-hunter wants to merge 4 commits into
Open
ashley-hunter wants to merge 4 commits into
ashley-hunter wants to merge 4 commits into
Conversation
A component id is `{path}@{ClassName}` and the path itself can contain `@`
(`node_modules/@scope/...`, `packages/@company/...`). Taking the text after
the first `@` produced update functions named after the path, e.g.
`function scope/pkg/src/a.ts_UpdateMetadata(...)`, which is a syntax error,
and made the vite plugin serve an empty HMR module for such components.
A class name is a JS identifier and cannot contain `@`, so the last `@` is
the separator. `compileForHmrSync` now uses the class name it is given
instead of re-parsing the id, matching Angular, which never parses ids.
The endpoint resolves the file path with `path.resolve`, which yields backslashes on Windows, so the tests must pass the same native path the other plugin tests use rather than a forward-slash one.
…lved path `componentsByFile` and `componentMetadataCache` are keyed by the transform id, which Vite gives with forward slashes on every platform. The endpoint looked them up by `path.resolve(fileId)`, which on Windows is a backslash path, so it served an empty module for every component and never treated an empty style list as definitive. Keep the resolved path for the disk reads only.
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.
Fixes HMR for components whose file path contains
@, such as scoped packages compiled from source (node_modules/@scope/...) or monorepo directories likepackages/@company/app/....The bug
A component id is
{path}@{ClassName}, and the class name was read back out as the text after the first@. When the path itself contains@, that produced an update function named after the path:which is a syntax error. The vite plugin had the same bug in two places (the
@ng/componentendpoint and the per-file class-name registration). There it showed up as an empty HMR response every time, with no error.The fix
A class name is a JS identifier and cannot contain
@, so the last@is the separator.generate_hmr_update_module_from_js:rsplit_once('@'), keeping theComponentfallback when there is no@.compileForHmrSync: uses the class name it is already given instead of re-parsing the id. This matches Angular, which names the function frommeta.classNameand never parses ids (@angular/buildlooks update modules up by the whole id).indexOf('@')→lastIndexOf('@')at both sites.parseComponentIdalready usedrfind.Also: the endpoint served nothing on Windows
While fixing the CI run on Windows this turned up a separate bug in the same endpoint.
componentsByFileandcomponentMetadataCacheare keyed by the transform id, which Vite gives with forward slashes on every platform (C:/Users/...). The endpoint looked them up bypath.resolve(fileId), which on Windows is a backslash path, so it:The existing tests passed
join()paths (backslashes on Windows), which matchpath.resolve, so they never saw it. The endpoint now looks the maps up byfileIdverbatim and keepsresolvedIdfor the disk reads only. A new test sends a forward-slash id; it failed on Windows CI before the fix (113f612) with only that test red.Tests
Each test was seen failing before its fix:
@in the path, and the no-@fallback.compileForHmrSync/generateHmrModule/parseComponentIdwith scoped paths; the generated module is checked to parse.packages/@company/..., and pruning a removed class under an@path leaves its sibling's pending update intact.transformAngularFilewithhmr: trueon a scoped path emits a parseable initializer containing the full encoded id.