Conversation
There was a problem hiding this comment.
Code Review
This pull request improves Sass package resolution caching in the esbuild stylesheet tool. It introduces a mechanism to cache and qualify package resolutions based on the visible node_modules directories of the importer rather than its exact file path. This allows stylesheets that share the same directory structure to reuse cached resolutions, while preventing incorrect cache hits when nested node_modules are present. Unit tests have been added to verify these caching behaviors. There are no review comments, and we have no additional feedback to provide.
c852690 to
f6d4a1c
Compare
There was a problem hiding this comment.
Thanks for investigating this and catching the cross-contamination issue with bare specifiers!
The bug you identified with d97c8857c is definitely real—unqualified caching across different package scopes (especially nested node_modules or conflicting versions) causes incorrect resolutions and negative caching issues (null).
However, I have concerns about using nodeModulesChainKey and manually crawling the filesystem:
- Yarn PnP / Virtual Stores: In Yarn PnP, physical
node_modulesdirectories do not exist on disk.existsSyncreturnsfalseat every level, collapsingchainKeyto""for all directories and regressing back to the globalurlcache. - Synchronous Filesystem I/O: Walking up the directory tree using
existsSyncintroduces synchronous I/O on the main thread. BecausenodeModulesChainCacheis cleared during watch rebuilds viaresetSassWorkerPoolCaches(), this synchronous crawl repeats on incremental rebuilds. - URL Scheme Safety:
fileURLToPath(options.containingUrl)will throw ifcontainingUrlis not afile:URL (e.g.,pkg:or custom schemes).
Suggested Alternative
Qualifying the cache key with resolveDir (e.g., ${resolveDir}:${url}) would fix correctness, but in standard Angular CLI apps where each component has its own folder (src/app/button/, src/app/card/), resolveDir differs for every component. That would largely revert the performance win of sharing package resolutions across components.
Instead, what do you think about distinguishing between project source files and third-party packages (node_modules)?
- For files within the project/workspace source tree (i.e. not inside
node_modules): All component stylesheets share the sametsconfig, workspace root, and top-levelnode_modules. We can qualify byworkingDirectoryso all components in the app continue to share package resolutions. - For files inside
node_modules: Qualify byresolveDirso dependencies with nestednode_modulesor different versions remain isolated.
Here is what that would look like:
// Inside findFileUrl:
const containingPath =
options.containingUrl?.protocol === 'file:'
? fileURLToPath(options.containingUrl)
: undefined;
const resolveDir = containingPath ? dirname(containingPath) : workingDirectory;
const isPackage = isPackageUrl(url);
// Files in node_modules are scoped to resolveDir to isolate nested dependency versions.
// Files in project source share workingDirectory so component stylesheets share resolutions.
const isNodeModules = /[\\/]node_modules[\\/]/.test(containingPath ?? '');
const scope = isNodeModules ? (resolveDir ?? '') : (workingDirectory ?? '');
const cacheKey = isPackage
? `${scope}:${url}`
: `${options.containingUrl?.href ?? ''}:${url}`;(and similarly for currentPackageRootCache using ${scope}:${packageName})
This eliminates the existsSync directory crawl, avoids the PnP and tsconfig path issues, and keeps package resolutions shared across application components while isolating dependencies.
f6d4a1c to
585bb25
Compare
|
@alan-agius4 thank you for catch - i hadn't considered it, especially pnp. Spec now cover dependency with own nested copy of package (both orders, negative cache, deep-import package root) and component stylesheets in different dirs still share one resolution. Mirrored same change to ng-packagr/ng-packagr#3438 |
585bb25 to
5afe6a0
Compare
…heets in node_modules Package specifiers were cached without any qualification, so the resolution made for one stylesheet was reused for every other stylesheet in the build. A dependency within `node_modules` that has its own nested version of a package received the version resolved for the application, the application received the nested version when the dependency was compiled first, and a failed resolution was reused for a dependency that is able to resolve the package. Which of these occurred depended on the order the stylesheets were compiled in. Package resolutions and package roots are now qualified with a scope. A stylesheet within `node_modules` uses its own directory as the scope, which keeps nested dependency versions isolated. All other stylesheets use the working directory of the build, so the component stylesheets of an application continue to share a single resolution. The scope is derived from the path of the stylesheet alone and requires no file system access. A containing URL that does not use the `file:` scheme is resolved from the working directory instead of causing an error.
5afe6a0 to
7d1383d
Compare
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Since d97c885 a bare Sass specifier is cached as
urlalone, so the resolution made for one stylesheet is handed to every other stylesheet in the build. Three outcomes, decided by the order the stylesheets compile in:nullresult is cached too, so a stylesheet that can resolve the package fails withCan't find stylesheet to import.An ordinary transitive version conflict is enough — a dependency carrying its own copy of a Sass package that the app also depends on. On a fixture like that,
ng buildon 22.2.0-rc.0 alternates between building and failing across clean runs of identical source, and on a successful run the wrong package version is in the emitted styles. Same fixture: 22.1.8 correct, 22.2.0-next.7 correct, 22.2.0-rc.0 wrong.The caches themselves being module-global (0a137f9) is not the problem — that commit kept both keys qualified.
Issue Number: N/A
What is the new behavior?
Package resolutions and package roots are qualified with a scope, as suggested in review. A stylesheet inside
node_modulesuses its own directory, which keeps a dependency's nested copy of a package isolated. Every other stylesheet uses the working directory of the build, so an application's component stylesheets still share one resolution. The scope comes from the stylesheet path alone, with no file system access, and a containing URL that is notfile:is resolved from the working directory instead of throwing.Over 900 stylesheets in 900 directories: 3 resolver calls and ~500 ms, the same as rc.0 as shipped, against 2700 calls and ~3.5 s for 22.1.8. One cost worth knowing about: because the importer now reads
containingUrl, Sass no longer reuses its own answers within a single compile, so a dependency with many subfolders importing the same package makes more resolver calls than on rc.0 (40 subfolders: 121 calls vs 4, 10.6 s vs 7.6 s; 22.1.8 took 9.1 s). Any importer-aware key has this cost. Against Angular Material 22.2.0-rc.0 over 300 stylesheets there is no measurable difference (14.37 s vs 14.35 s).Specs cover a dependency with its own nested copy of a package in both compile orders, a failed lookup not reused by a stylesheet that can resolve the package, a deep import not using another scope's package root, component stylesheets in different folders sharing a single resolution, and a non-
file:containing URL. Each spec fails when the part of the change it covers is reverted.bazel test //packages/angular/build:testpasses.By design, a project-source folder with its own nested
node_modulesshares the application scope, as it did on rc.0.Only the rc is affected; no stable release has the regression. Related: #34117 makes these caches survive more watch rebuilds.
The same change for ng-packagr is in ng-packagr/ng-packagr#3438.
Does this PR introduce a breaking change?