Fix #1034: Improve symlink resolution in module specifier generation #1902
Conversation
|
My intention is to help fix #1034, which is the only thing left blocking my team from adopting tsgo. I'm not very experienced with Go, but I wanted to expand upon @shinichy's work here: https://github.com/shinichy/typescript-go/tree/symlinks |
|
We have the same issue in our team that's stopping us from adopting So I compiled your branch and ran a test, it doesn't seem to make any difference in our code base. Just wanted to let you know. |
Extends the symlink support in GetEachFileNameOfModule to properly resolve module specifiers across symlinked packages and workspaces. Key changes: - Move knownsymlinks from compiler to dedicated symlinks package - Implement active resolution via ResolveModuleName to populate cache - Add dependency resolution from package.json to detect symlinks early - Improve ignored path handling (node_modules/., .git, .# emacs locks) - Add comprehensive test coverage for symlink resolution - Fix declaration emit to prefer original paths over symlink paths This aligns with upstream TypeScript's symlink resolution behavior, ensuring correct module specifiers in declaration files for monorepos and symlinked dependencies. Fixes baseline mismatches in: - declarationEmitReexportedSymlinkReference2/3 - symlinkedWorkspaceDependencies* tests - nodeModuleReexportFromDottedPath
Optimizes populateSymlinkCacheFromResolutions to avoid redundant dependency resolution. Previously, every module specifier generation would re-resolve all package.json dependencies. Now uses package-level caching to resolve once and reuse results. Performance improvements (measured with benchmarks): - Speed: 9.28x faster (89.2% reduction: 509µs → 55µs per operation) - Memory: 8.64x less (88.4% reduction: 597KB → 69KB) - Allocations: 9.22x fewer (89.2% reduction: 12,177 → 1,321) Key changes: - Add package-level cache tracking in KnownSymlinks - Eliminate intermediate slice allocations - Reduce redundant ToPath() calls - Add comprehensive benchmarks for symlink operations For a project with 50 dependencies and 100 files, this saves multiple seconds of compilation time by avoiding 5,000+ redundant resolutions.
Interesting. This is what I get in Linux after setting up your repo and replacing Seems like this might need some fixes for macOS. |
|
@neo773 should be fixed with the latest commit, the above errors seem like they might be actual errors or at least ones that can be fixed trivially: Patch
diff --git a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/services/imap-message-text-extractor.service.ts b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/services/imap-message-text-extractor.service.ts
index f81f151..335fe77 100644
--- a/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/services/imap-message-text-extractor.service.ts
+++ b/packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/services/imap-message-text-extractor.service.ts
@@ -8,10 +8,12 @@ import * as planer from 'planer';
import { safeDecodeURIComponent } from 'src/modules/messaging/message-import-manager/drivers/imap/utils/safe-decode-uri-component.util';
+type DOMPurifyInstance = ReturnType<typeof DOMPurify>;
+
@Injectable()
export class ImapMessageTextExtractorService {
private readonly jsdomInstance: JSDOM;
- private readonly purify: DOMPurify.DOMPurify;
+ private readonly purify: DOMPurifyInstance;
constructor() {
this.jsdomInstance = new JSDOM('');
diff --git a/packages/twenty-server/src/utils/image.ts b/packages/twenty-server/src/utils/image.ts
index 8e98729..ad78167 100644
--- a/packages/twenty-server/src/utils/image.ts
+++ b/packages/twenty-server/src/utils/image.ts
@@ -1,4 +1,4 @@
-import { type Axios } from 'axios';
+import { type AxiosInstance } from 'axios';
const cropRegex = /([w|h])([0-9]+)/;
@@ -24,7 +24,7 @@ export const getCropSize = (value: ShortCropSize): CropSize | null => {
export const getImageBufferFromUrl = async (
url: string,
- axiosInstance: Axios,
+ axiosInstance: AxiosInstance,
): Promise<Buffer> => {
const response = await axiosInstance.get(url, {
responseType: 'arraybuffer',
diff --git a/packages/twenty-server/test/integration/metadata/suites/field-metadata/morph-relation/failing-field-metadata-morph-relation-creation.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/field-metadata/morph-relation/failing-field-metadata-morph-relation-creation.integration-spec.ts
index 0aa4bb7..2981c77 100644
--- a/packages/twenty-server/test/integration/metadata/suites/field-metadata/morph-relation/failing-field-metadata-morph-relation-creation.integration-spec.ts
+++ b/packages/twenty-server/test/integration/metadata/suites/field-metadata/morph-relation/failing-field-metadata-morph-relation-creation.integration-spec.ts
@@ -1,4 +1,4 @@
-import { faker } from '@faker-js/faker/.';
+import { faker } from '@faker-js/faker';
import { createOneFieldMetadata } from 'test/integration/metadata/suites/field-metadata/utils/create-one-field-metadata.util';
import { createOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/create-one-object-metadata.util';
import { deleteOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/delete-one-object-metadata.util'; |
|
@chase Thank you for your work on this. |
|
Thanks for working on this! Fixes #1034 (comment) and #1657 also |
chase
changed the title
internal/compiler/program.go
Outdated
| // if p.Host().GetSymlinkCache() != nil { | ||
| // return p.Host().GetSymlinkCache() | ||
| // } | ||
| if p.knownSymlinks == nil { |
There was a problem hiding this comment.
This condition looks to be impossible as-written, but I think lazy initialization is a good idea. However, you need to guard the field initialization with a sync.Once like the other lazy computed caches.
internal/compiler/program.go
Outdated
| // In declaration-only builds, the symlink cache might not be populated yet | ||
| // because module resolution was skipped. Populate it now if we have resolutions. |
There was a problem hiding this comment.
I don’t recall exactly how this happened in Strada, but I don’t think this comment applies in Corsa.
There was a problem hiding this comment.
This occurs regularly in pnpm workspaces when running tsgo --build --emitDeclarationsOnly, if I recall correctly.
| // Helper to resolve dependencies without creating intermediate slices | ||
| resolveDeps := func(deps map[string]string) { | ||
| for depName := range deps { | ||
| resolved := host.ResolveModuleName(depName, packageJsonPath, options.OverrideImportMode) |
There was a problem hiding this comment.
I’d like to get this out of here by frontloading these resolutions to program construction. That will allow us to remove ResolveModuleName from the host interfaces and should also remove all concurrent writes from the symlink cache. In moving it there, I also think the program could do a better job of determining which of these resolutions are necessary and avoid doing redundant ones. The original comment that said most of these resolutions will already be cached is not currently true in Corsa.
It is likely the case that we get 90% of the way there without this block of logic, as this was added to Strada long after the symlink cache existed with only the resolutions it made during program construction. Dropping it from this PR and leaving it as a to-do would be fine. The priority is to eliminate file system reads and cache mutations post-program-load.
There was a problem hiding this comment.
I'm worried that since the current test suite don't adequately cover handling pnpm workspaces, that removing this and relocating some of this willl cause a regression for some of the cases that were solved for others.
I might have time to work on this over the weekend, but if your or another wants to pick up this branch and get it up to standard, I don't want to to block getting a fix merged.
|
Unfortunately I won't be able to continue working on this for the foreseeable future due to some unforeseen life events. I'm not sure if this should be closed, but I'd be happy if someone else could pick this up or build from this while I'm away from keyboard. |
internal/compiler/program.go
Outdated
| if len(result.resolvedModules) > 0 || len(result.typeResolutionsInFile) > 0 { | ||
| result.knownSymlinks.SetSymlinksFromResolutions(result.ForEachResolvedModule, result.ForEachResolvedTypeReferenceDirective) | ||
| } | ||
| p.populateSymlinkCacheFromResolutions() |
There was a problem hiding this comment.
I think we should just copy the symlink cache on program clone. I think it's intended to be immutable after initialization, so by copy, I mean the new program can share the same reference. (If the cache were mutable, we would have to add a clone method on it.) This function needs to be dirt cheap, and calling it means you're asserting that the only relevant change that's happened in the Program's whole world is to the file you're passing in.
|
This should be marked as fixing #1034, yes? |
|
About to push an update; here’s my response to my own suggestions:
Done, and checked that this has no measurable performance impact by switching
It’s not quite that simple, because the resolution of a package name can vary based on the location we’re starting from. For my original suggestion to work, we’d have to store not only the package name, but also the requesting location and the resolved location, and then do some calculation based on those later to figure out whether that resolution holds for the package.json location we’re interested in. That didn’t seem worth either the code complexity or allocations. However, we can do almost as well for free by checking to see whether the symlink cache already has an entry for a given dependency in the node_modules directory adjacent to the package.json. If so, we can skip the resolution and realpath call, because the resolution is guaranteed to resolve to that first node_modules directory if it exists.
@iisaduan did this, so I started to remove the SyncMaps and SyncSets, but then I noticed that |
| +//// [keys.d.ts] | ||
| +import { MetadataAccessor } from "@raymondfeng/pkg2"; | ||
| +export declare const ADMIN: MetadataAccessor<boolean, import("../../pkg1/dist").IdType>; | ||
| +export declare const ADMIN: any; |
There was a problem hiding this comment.
I was going to ask why this regressed, but this file shouldn't be emitted anyhow
jakebailey
dismissed
andrewbranch’s stale review
Pushed to this PR, can't review it now :)
|
I think I expected the PR to be fixing a bunch of other symlink related issues in the fourslash tests, so I'm not sure what else we are missing. Maybe it's the stuff @johnfav03 has commented out in #1793? |
|
My auto-imports rewrite is not going to go through the same code path, so I’m not sure if it matters much right now. Can you give me an example of a test you’re thinking of? |
|
https://github.com/microsoft/typescript-go/blob/bd7c18dc8ed3c5ed960d72f1e329353f0a594bcc/internal/fourslash/tests/gen/autoImportCrossProject_symlinks_toSrc_test.go or similar, the tests I skipped when I added auto import fixes |
|
@jakebailey all those also rely on the package.json auto import provider, so they never even try to pull on the symlink cache. Those are blocked on my auto-imports rewrite (which is going to have to deal with symlinks separately) |
|
Hm, this ended up failing all of the tests. |
|
Oops, I committed the removal of |
|
Seems like three tests are being fixed: And linting needs to be fixed. |
|
Thank you @andrewbranch and @iisaduan for bringing this over the finish line while I get my ducks back in a row. |
|
Really appreciate you all guys!! It would be wonderful innovation for pnpm ecosystem 🫶 |
8 participants
Extends the symlink support in GetEachFileNameOfModule to properly resolve module specifiers across symlinked packages and workspaces.
Key changes:
Fixes #1657
Fixes #1034
Fixes #1347