-
Notifications
You must be signed in to change notification settings - Fork 719
support pnp resolver #1876
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?
support pnp resolver #1876
Conversation
logger.Log(fmt.Sprintf("ATA:: Installed typings %v", packageNames)) | ||
var installedTypingFiles []string | ||
resolver := module.NewResolver(ti.host, &core.CompilerOptions{ModuleResolution: core.ModuleResolutionKindNodeNext}, "", "") | ||
resolver := module.NewResolver(ti.host, &core.CompilerOptions{ModuleResolution: core.ModuleResolutionKindNodeNext}, "", "", nil) |
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.
todo: add pnp support later
packageName, rest := ParsePackageName(moduleName) | ||
packageDirectory := tspath.CombinePaths(nodeModulesDirectory, packageName) | ||
|
||
func (r *resolutionState) loadModuleFromSpecificNodeModulesDirectory(ext extensions, candidate string, packageDirectory string, rest string, nodeModulesDirectoryExists bool) *resolved { |
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.
I change this function's interface
} | ||
|
||
// need fixtures to be yarn install and make global cache | ||
func TestGlobalCache(t *testing.T) { |
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.
These test cases were taken from pnp-rs, but since they are relatively complex to run and maintain.
I think it would be fine to remove them if they’re not considered necessary.
Please let me know your thoughts!
@microsoft-github-policy-service agree |
pnpResolutionConfig := TryGetPnpResolutionConfig(currentDirectory) | ||
|
||
if pnpResolutionConfig != nil { | ||
fs = pnpvfs.From(fs) |
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.
On second thought, this way I wouldn’t be able to take advantage of the cached VFS, so I’m thinking of moving its location instead.
You said some of this comes from pnp-rs. Is it a pure port? What is the license of that code? Do you own it? (You signed the CLA, but that may not be enough if it's not yours.) |
@jakebailey As far as I understand, this means I need to include the original author’s license and copyright notice in the comments. Would this cause any issues? If everything is fine, I’ll go ahead and add the appropriate BSD 2-Clause License notice. |
We could pull it, but would need to explicitly declare that dependence in some extra metadata to generate NOTICE.txt (something we have avoided entirely by ensuring all code was written by us from spec or tests, but it's not really different than other deps). But you definitely need to declare that there are files derived from code under a different license. |
@jakebailey |
No, the files containing the code need to have headers with said license. For example, these tests:
|
Thank you. I’ve added the license notice as suggested. @jakebailey |
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.
A "few" comments; my overall worry is that the code style is not so much similar to our code, but maybe that's okay.
(My reviewing it is not really saying yes or no to the PR, we haven't discussed it)
"strings" | ||
) | ||
|
||
func NormalizePath(original string) string { |
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.
I would think we don't need this sort of thing; the tspath
package already defines the way we typically normalize paths, which is already forward slashed and I think compatible with accessing zips.
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.
305fd49
I replaced normalizePath with tspath.normalizePath.
var ( | ||
reWindowsPath = regexp.MustCompile(`^([a-zA-Z]:.*)$`) | ||
reUNCWindowsPath = regexp.MustCompile(`^[\/\\][\/\\](\.[\/\\])?(.*)$`) | ||
rePortablePath = regexp.MustCompile(`^\/([a-zA-Z]:.*)$`) | ||
reUNCPortablePath = regexp.MustCompile(`^\/unc\/(\.dot\/)?(.*)$`) | ||
) |
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.
Using regexes is definitely not going to be a good idea; these are notoriously slow. I would hope we could avoid needing any of this.
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.
305fd49
I removed this file as normalizePath has been removed.
} | ||
|
||
func fromPortablePath(s string) string { | ||
if runtime.GOOS != "windows" { |
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.
Depending on runtime.GOOS
is a bad idea; all of our other path handling is platform independent.
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.
305fd49
I removed this file as normalizePath has been removed.
files := make([]string, 0) | ||
dirs := make([]string, 0) |
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.
make([]string, 0)
is just an allocate-y version of var files []string
; don't intentionally make an empty slice unless nil
vs non-nil
matters.
module github.com/microsoft/typescript-go | ||
|
||
go 1.25 | ||
go 1.25.1 |
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.
Please undo this.
return "", false | ||
} | ||
|
||
var rePNP = regexp.MustCompile(`(?s)(const[\ \r\n]+RAW_RUNTIME_STATE[\ \r\n]*=[\ \r\n]*|hydrateRuntimeState\(JSON\.parse\()'`) |
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.
This scares me a bit; because of the regex, but also because this reads JS files? Does the spec really require such a thing?
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.
Good point! esbuild uses the JS AST for parsing. Since I was primarily focused on porting pnp-rs, I initially brought the code over as-is, but as you mentioned in another comment, there are definitely some risky areas. I believe the TypeScript Go codebase also includes a JS parser, so I’ll try using that for parsing instead.
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.
According to the official spec, in a Node.js environment, pnp.cjs patches Node’s module resolution, so it’s executed first to override the default behavior. In Go, however, this part needs to be implemented manually.
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.
Ultimately, parsing pnp.cjs is necessary to read the metadata, so this part seems unavoidable.
If we parse it using an AST like esbuild does, it might be a bit more reliable — would that be okay?
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.
That makes more sense to me. Though in general I really thought that PnP had been specified to just emit a plain JSON file...
internal/module/pnp/manifest.go
Outdated
} | ||
|
||
func (r *RegexDef) compile() (*regexp.Regexp, error) { | ||
return regexp.Compile(r.Source) |
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.
This will fail because regex
is re2 syntax, not ECMAScript. You'd have to use regexp2
in ECMAScript mode.
That being said, is this really ever used? Does the PnP spec really require regex parsing? (How do esbuild and so on do this without pulling on totally different regex implementations?)
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.
Thanks for the feedback!
The original ignorePatternData
looks like this:
"ignorePatternData": "(^(?:\\\\.yarn\\\\/sdks(?:\\\\/(?!\\\\.{1,2}(?:\\\\/|$))(?:(?:(?!(?:^|\\\\/)\\\\.{1,2}(?:\\\\/|$)).)*?)|$))$)",
Esbuild also compiles and uses the regex in the same way. Since this is part of the Yarn PnP spec, it seems unavoidable to use a regular expression here.
For now, I’ve optimized this commit to avoid the inefficiency of compiling the regex on every use. f0ee55b
@jakebailey Thank you for the thoughtful review! I know feedback like this takes a lot of time, and I truly appreciate it. Reading your comments made me realize how unfamiliar I still am with the TypeScript Go codebase—I’m sorry I didn’t reference the existing code more. Consistent code style is especially important in open source, so I’m happy to adopt all of your suggestions. I’ll start by addressing your comments. |
This PR adds support for PnP resolution.
#1875 in the previous discussion, I have internalized pnp-go into the repository.
Please note that the PR has become quite large due to the inclusion of the PnP source and test code. I apologize for the length and appreciate your understanding.
what is pnp
Yarn Plug’n’Play (PnP) is a dependency resolution system that removes the need for a traditional node_modules folder.
describe at #460
how to support
add pnp resolution config in host
add pnp branch in resolver.go