Skip to content
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

[Feature] PnP resolveToUnqualified() should return an array #1927

Closed
2 tasks
pauldraper opened this issue Oct 4, 2020 · 8 comments
Closed
2 tasks

[Feature] PnP resolveToUnqualified() should return an array #1927

pauldraper opened this issue Oct 4, 2020 · 8 comments
Labels
enhancement New feature or request waiting for feedback Will autoclose in a while unless more data are provided

Comments

@pauldraper
Copy link

pauldraper commented Oct 4, 2020

  • I'd be willing to implement this feature
  • This feature can already be implemented through a plugin

Describe the user story

Node.js

PnP is incapable of describing the Node.js resolution algorithm. Namely, the primary function resolveToUnqualified() cannot support a list of directories.

Resolving module 'exam/ple':

/home/me/dir/node_modules/exam/ple
/home/me/node_modules/exam/ple
/home/node_modules/exam/ple
/node_modules/exam/ple
/usr/lib/node_modules/npm/node_modules/exam/ple
<internal>

Currently, to accommodate Node.js resolution, PnP allows the package manager to be agnostic about extension, delegating to the module resolver to use file system info to determine that. However, to match Node.js, it also needs to let the package manager be flexible about directory.

Admittedly, it's unconventional Node.js to have a file in both:

/home/me/dir/node_modules/exam/ple.js
/usr/lib/node_modules/npm/node_modules/exam/ple2.js

Bazel

Also, when working to create a PnP API for the build tool/dependency manager Bazel, I run into this same issue.

BUILD.bazel

js_library(
  name = "js",
  package = "example",
  deps = [":gen"],
  src = glob(["**/*.js"])
)

generate_js(
  name = "gen",
  package = "example",
)

# irrelevant, but to illustrate they are treated as part of same "package"
npm_publish(
  files = [":gen", ":js"],
  name = "npm",
  package = "example",
  version = "1.0.0",
)

foo.js

import { Bar } from './bar';
new Bar();

bazel-out/blah/blah/blah/gen/bar.js (generated)

import { Bar } from './bar';
new Bar();

The package manager Bazel doesn't know which directory the './bar' module is in. It needs to tell the runtime to check in both.

Describe the solution you'd like

I'd like resolveToUnqualified() (or new function resolveToUnqualifiedList()) to return a list.

Describe the drawbacks of your solution

Backwards compatibility.

Describe alternatives you've considered

Have the package manager be aware of the complete resolution, that is, use qualified paths.

Additional context

@pauldraper pauldraper added the enhancement New feature or request label Oct 4, 2020
@arcanis
Copy link
Member

arcanis commented Oct 4, 2020

There is a single resolution for a single module and bare identifier, so resolveToUnqualified semantically cannot return an array.

@pauldraper
Copy link
Author

pauldraper commented Oct 4, 2020

There is a single resolution for a single module and bare identifier

That's true of resolveRequest(), but PnP has chosen to have a resolveToUnqualified() function with a permitted varying extension.

What you call "single resolution" is in reality a patterned file path with many possible values.

I argue that resolveToUnqualified() under-captures the amount of runtime variability in path that Node.js algorithm requires.

@arcanis
Copy link
Member

arcanis commented Oct 4, 2020

So, resolveToUnqualified doesn't permit varying extension ... in itself. In fact, it doesn't care at all about the extension. It does a single thing: statically resolve a bare identifier to the location where the package lives.

In the case of Node it's a bit problematic because Node doesn't have any concrete idea what a package is. If you import foo/bar and foo/baz from the same module, in Node-land you might end up importing from different folders ... this is fairly stupid (especially when combined to the hoisting), and doesn't work this way in PnP: one bare identifier in a module means one location, period.

I argue that resolveToUnqualified() under-captures the amount of runtime variability in path that Node.js algorithm requires.

We don't use it to represent the Node resolution, though 😉 That said, it can represent a "good enough" subset of the Node resolution, if you keep the requirement I mention (which is easy enough - I'm not aware of a single case where keeping the problematic behavior matters in practice).

@arcanis arcanis added the waiting for feedback Will autoclose in a while unless more data are provided label Oct 4, 2020
@pauldraper
Copy link
Author

pauldraper commented Oct 5, 2020

and doesn't work this way in PnP: one bare identifier in a module means one location

Yes, that is how it works now. No argument there.

I argue that Node.js, TypeScript*, and my Bazel code gen situation wind up with "split packages," so PnP should support that.

My package manager/build system has modules in src/* and generated modules in gen-src/*. When asked via PnP API where a bare identifier lives, it has no good answer, at least not without inspecting files and understanding the file extension algorithms.

I could of course merge split packages, but by the same reasoning I also could run a node_modules linker. This is something that I'd hope the location-flexible PnP API would support.


* I should have added TypeScript to the list of tools whose module resolution cannot be described with PnP. The entire purpose of TypeScript's roots option if for split packages/module roots. It works today only because TS first runs an internal module resolution outside PnP.

@arcanis
Copy link
Member

arcanis commented Oct 5, 2020

I guess I should ask one question first: why does it matter whether the PnP API can perfectly represent a loose resolution like the Node one? We don't use it for the Node resolution. Do you have a reproducible example?

@pauldraper
Copy link
Author

pauldraper commented Oct 5, 2020

I thought PnP might be a good commonly supported abstraction for resolving JS, CSS, etc. modules for Bazel (which kinda runs the show with where files go).

Sometimes the split package case comes up, similar to TypeScripts's rootDirs option.

If PnP keeps one unqualified path per module identifier (and it sounds like that is the conclusion), I will instead write require.resolve shims, Webpack resolver, etc. to accomplish my intention.

@yarnbot
Copy link
Collaborator

yarnbot commented Nov 4, 2020

Hi! 👋

It seems like this issue as been marked as probably resolved, or missing important information blocking its progression. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it.

@yarnbot yarnbot added stale Issues that didn't get attention and removed stale Issues that didn't get attention labels Nov 4, 2020
@merceyz
Copy link
Member

merceyz commented Nov 4, 2020

Closing since resolveToUnqualified can't return an array as that doesn't make sense here. rootDirs seems like a way to use workspaces without using workspaces, at which point you should probably just use workspaces.

@merceyz merceyz closed this as completed Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting for feedback Will autoclose in a while unless more data are provided
Projects
None yet
Development

No branches or pull requests

4 participants