feat: Package Json Resolver as trait #27764
Conversation
Signed-off-by: Benjamin Swerdlow <[email protected]>
There was a problem hiding this comment.
Can you explain exactly why you need this to be a trait in the PR description? I'm not sure this is necessary.
There was a problem hiding this comment.
I want to swap out the Package Json Resolver with a virtual one that supports other formats
There was a problem hiding this comment.
In particular, for virtual node modules directories, I also want to be able to make it work with stuff like pnpm systems.
There was a problem hiding this comment.
Is it loading that needs to be configured? Can you change that behaviour with the sys implementation? Is there some docs on how those systems work?
|
Have my own sys implementation already. I just have a bunch of things I
want to change about this for weird use cases. Another thing is that it
automatically goes all the way up my file system checking for package jsons
at every place, which in projects without package jsons can cause actual
feeling of slowness of launch vs it not being there.
https://www.linkedin.com/in/benswerdlow/
…On Thu, Jan 23, 2025 at 7:36 AM David Sherret ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
On cli/lib/loader.rs
<#27764 (comment)>:
Is it loading that needs to be configured? Can you change that behaviour
with the sys implementation? Is there some docs on how those systems work?
—
Reply to this email directly, view it on GitHub
<#27764 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJEI4PWKUZQWTKEC5K4KAS32MEEAFAVCNFSM6AAAAABVTT7PK2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNZQGIZTCMRXGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I'd also argue this change brings the structure of the PackageJsonResolver more inline with the rest of the NodeResolver. It is the outlier in both constructing NodeExtInitServices and also in the NodeResolver struct itself, where the other services like I think the changes I made also overall simplify the code by removing the need for the Generic in many places, so you can just pass any PackageJsonResolver you want in. Those are both arbitrary opinions, just FYC. One of the reasons I started this refactor is because the PackageJsonResolver has an unwrap in it. — if you are trying to run in root directory, it immediately looks to the parent of that path (which is none), and panics. TSys changes cannot fix this. This makes sense for deno's use case, but I don't like that I cannot re-implement this inline with some custom behaviors I want — for example using an arbitrary store for where the package.json's are so I don't need to do this lookup. |
This makes the code slower by using dynamic dispatch, which is part of the reason I don't want to land this.
There is a store for the package.json. I'm thinking we should make this configurable instead because it's easy to do. We should wait for #27766 to land though.
Sounds like we should fix that bug instead if it is indeed a bug. That said, it sounds like something is passing a directory instead of a file path though so it might be a bug elsewhere. Have you been able to see this bug in Deno?
This is necessary for node resolution. Once it discovers there is no longer a package.json in any ancestor directory then it stops searching. I'm not sure there's any need to make this configurable because it adds more complexity to Deno and this crate is about implementing node resolution. If there is something wrong with how it is doing node resolution then we should fix that, but I don't think making stuff configurable that's not used in Deno is worth the increased maintenance cost. |
|
Gimme an hour, lemme see if I can get a swap to impl working |
|
Tried a bunch of refactors and it was too much, I think this might have to wait until impl in type declarations works. Damn. |
|
Yeah, I don't think we'd want to add more type parameters unless we need it. It's already quite out of control. I think though that this should serve your needs: #27800 -- Using this you can provide any package.json for the provided specifier. |
Package Json Resolver Trait
Made PackageJson Resolver a trait that can be implemented with any behavior in the node resolver, so I can swap out the package json to support other formats.