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

(PnP) modules in tsconfig.json#extends aren't properly resolved #2456

Closed
jonaskuske opened this issue Aug 11, 2022 · 10 comments
Closed

(PnP) modules in tsconfig.json#extends aren't properly resolved #2456

jonaskuske opened this issue Aug 11, 2022 · 10 comments

Comments

@jonaskuske
Copy link
Contributor

Now that esbuild supports PnP, extending a tsconfig from an installed package should work and this issue should be fixed: vitejs/vite#8357

However, it doesn't quite work yet. If you manually resolve the path using require.resolve and then set the resolved virtual path as "extends", everything is fine and the JSON is read from its zip archive:

// ✅
{ "extends": "./.yarn/__virtual__/@vue-tsconfig-virtual-084478b2ba/0/cache/@vue-tsconfig-npm-0.1.3-07ae9b676e-8150a24497.zip/node_modules/@vue/tsconfig/tsconfig.json" }

But with a bare module specifier, esbuild fails to load the tsconfig.json and prints a warning.

// ❌
{ "extends": "@vue/tsconfig/tsconfig.json" }

So I assume somewhere a resolve step is missing?

@evanw
Copy link
Owner

evanw commented Aug 11, 2022

This is a Vite problem, not an esbuild problem. Vite doesn't use esbuild for bundling so it's impossible for esbuild's new PnP support to do anything here. Closing.

@evanw evanw closed this as not planned Won't fix, can't repro, duplicate, stale Aug 11, 2022
@jonaskuske
Copy link
Contributor Author

As someone in the Vite issue thread pointed out, the config file is bundled directly using esbuild.build: https://github.com/vitejs/vite/blob/e53c029ef645d1d91c284799489a11d1f41303ec/packages/vite/src/node/config.ts#L934

(and PnP support in esbuild 0.15 does have an effect, when specifying the resolved path esbuild 0.14 still fails because it can't read the zip archive but 0.15 handles it no problem)

But you're right, I should maybe try to create a minimal repro independent from Vite 😅

@swandir
Copy link

swandir commented Aug 11, 2022

so it's impossible for esbuild's new PnP support to do anything here

Seems like it would be possible If esbuild were to use the same resolution mechanism for tsconfig's extend field (according to the docs the path there may use Node.js style resolution).

@evanw
Copy link
Owner

evanw commented Aug 11, 2022

Sorry, my mistake. I did not realize Vite was using esbuild as a bundler like this.

@evanw evanw reopened this Aug 11, 2022
@jonaskuske
Copy link
Contributor Author

@evanw
Copy link
Owner

evanw commented Aug 12, 2022

The problem here is that package path resolution depends on tsconfig.json because of the baseUrl and paths features, but package path resolution is required to handle extends in tsconfig.json. Using esbuild's normal path resolution would introduce an infinite loop. To break the cycle, esbuild hard-codes a limited form of path resolution just for extends in tsconfig.json that bypasses the path resolution cache. So I can make PnP modules work for extends in tsconfig.json but it will impose some performance overhead for everyone that uses tsconfig.json with extends.

@swandir
Copy link

swandir commented Aug 12, 2022

So I can make PnP modules work for extends in tsconfig.json but it will impose some performance overhead for everyone that uses tsconfig.json with extends.

Is it everyone using PnP or just everyone?

If it's the former, then I guess performance overhead is better that not working 🧐

@evanw
Copy link
Owner

evanw commented Aug 12, 2022

Everyone, since esbuild will have to search for PnP manifests to see if they exist. This will also be in the serial part of the build before most of the parallelism kicks in because package path resolution is still being set up.

@swandir
Copy link

swandir commented Aug 12, 2022

Would an opt-in via an option be better?

And perhaps ESBuild's NPM wrapper could check for Yarn's PnP register/loader in NODE_OPTIONS and turn that option on. Kind of the same as with Node.js itself — it has to be run via yarn node for PnP to work.

UPD: or check for process.versions.pnp

@evanw
Copy link
Owner

evanw commented Aug 12, 2022

it has to be run via yarn node for PnP to work

This is not true for esbuild since esbuild is written in native code instead of JavaScript. Requiring this would be a huge performance penalty because Yarn is very slow to start up, so I'm not going to do this.

Would an opt-in via an option be better?

I'm just going to implement this for everyone. The performance cost probably isn't big.

@evanw evanw closed this as completed in 4e68c27 Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants