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

rpt2 does not resolve pnpm symlinked dependencies #234

Closed
davelsan opened this issue Jul 21, 2020 · 17 comments · Fixed by #332
Closed

rpt2 does not resolve pnpm symlinked dependencies #234

davelsan opened this issue Jul 21, 2020 · 17 comments · Fixed by #332
Assignees
Labels
kind: bug Something isn't working properly topic: monorepo / symlinks Related to monorepos and/or symlinks (Lerna, Yarn, PNPM, Rush, etc) topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs

Comments

@davelsan
Copy link

davelsan commented Jul 21, 2020

What happens and why it is wrong

When trying to build a project installed with pnpm, rpt2 fails to resolve the created symlinked dependencies in node_modules (non-flat).

Setting check: false is (arguably) a workaround.

Related issues: #188, #214

Reproduction

Repository: https://github.com/davelsan/rpt2-pnpm-symlinks
Config: https://github.com/davelsan/rpt2-pnpm-symlinks/blob/master/rollup.config.js

The provided repro is a minimal vue3 plugin that imports the App interface from vue in index.ts

import  { App } from 'vue'

This import causes the following error on build:

 [!] (plugin rpt2) Error: semantic error TS2305: Module '"../node_modules/vue/dist/vue"' has no exported member 'App'.

Setting the check option to false in rollup.config.js eliminates the error.

Versions

  • typescript: 4.2.3
  • rollup: 2.41.5
  • rollup-plugin-typescript2: 0.30.0

rollup.config.js

import path    from 'path';
import rpt2    from 'rollup-plugin-typescript2';
import pkgJson from './package.json';

export default {

  input: './src/index.ts',

  external: ['vue'],

  plugins: [
    rpt2({
      check: true,
      tsconfig: path.resolve(__dirname, 'tsconfig.json'),
    })
  ],

  output: [
    {
      file: pkgJson.browser, // dist/index.esm.js
      format: 'es',
      sourcemap: true,
    },
  ],
}

tsconfig.json

A snippet with the relevant module resolution options only.

{
  "compilerOptions": {
    "target": "esnext",
    "module": "esnext",
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
  }
}

package.json

A snippet with the relevant dependencies only.

{
  "peerDependencies": {
    "vue": "^3.0.7"
  },
  "devDependencies": {
    "rollup": "^2.41.5",
    "rollup-plugin-typescript2": "^0.30.0",
    "typescript": "^4.2.3",
    "vue": "^3.0.7"
  }
}
@davelsan
Copy link
Author

davelsan commented Jul 21, 2020

Update

According to this comment in the pnpm repo, there was a similar problem caused by the rollup-plugin-commonjs plugin (note that I'm not using it in the repro).

However, none of the provided solutions will work in this case.

// include the actual pnpm registry path
commonjs({ include: '../../node_modules/.pnpm/registry.npmjs.org/**' })

// or set the following option in rollup.config.js
preserveSymlinks: true

@davelsan
Copy link
Author

davelsan commented Jul 23, 2020

I've been looking at the code and I'm mystified. Leaving more info here for myself and other passing-by folks:

const resolved = pluginOptions.rollupCommonJSResolveHack
? resolve.sync(result.resolvedModule.resolvedFileName)
: result.resolvedModule.resolvedFileName;

Based on the above, I cannot see any combination of options and/or plugins that will correctly resolve pnpm symlinks. Maybe I could try to implement it? It looks simple enough, which probably means it is more complicated than I can see 😅

@davelsan
Copy link
Author

I'm starting to suspect this is not an issue with rpt2, despite what the error says, but rather a problem that's propagating upstream.

Closing until I have more information.

@theoparis
Copy link

theoparis commented Mar 3, 2021

Any updates on this? I'm trying to bundle a react app with rollup and the original @rollup/plugin-typescript gives debug failure errors, so I tried this.

I got the following error specifically:

[!] (plugin rpt2) Error: /media/theo/TheoData/Essentials/Projects/Programming/hypervpn/src/pages/home.tsx(1,10): semantic error TS2305: Module '"@chakra-ui/react"' has no exported member 'Button'.
src/pages/home.tsx
Error: /media/theo/TheoData/Essentials/Projects/Programming/hypervpn/src/pages/home.tsx(1,10): semantic error TS2305: Module '"@chakra-ui/react"' has no exported member 'Button'.
    at error (/media/theo/TheoData/Essentials/Projects/Programming/hypervpn/node_modules/.pnpm/rollup@2.40.0/node_modules/rollup/dist/shared/rollup.js:5279:30)

@theoparis
Copy link

Thing is, it works fine with the original ts plugin before I had debug failure errors, and running tsc by itself works fine as well.

@beetaa
Copy link

beetaa commented Mar 17, 2021

Thing is, it works fine with the original ts plugin before I had debug failure errors, and running tsc by itself works fine as well.

I am using pnpm as the package manager, same situation while rolling up a vue component. no problem when using @rollup/plugin-typescript.

image

@beetaa
Copy link

beetaa commented Mar 17, 2021

@davelsan @creepinson I think this issue should be opened again for further discussion. thank you.

@davelsan
Copy link
Author

Hi @beetaa. Thanks for your interest in this issue. I have updated the original repro and I can confirm the issue is still there. I'm re-opening the discussion as per your request.

@davelsan davelsan reopened this Mar 18, 2021
@ezolenko
Copy link
Owner

@davelsan

This plugin does use resolve.sync if rollupCommonJSResolveHack is set to true. But it does not pass an options object.
[...]
Based on the above, I cannot see any combination of options and/or plugins that will correctly resolve pnpm symlinks. Maybe I could try to implement it? It looks simple enough, which probably means it is more complicated than I can see 😅

So you are saying rpt2 should pass { preserveSymlinks: false } option to resolve?

@jackson-yyy
Copy link

jackson-yyy commented Aug 17, 2021

[... shortened for readability ...]
So you are saying rpt2 should pass { preserveSymlinks: false } option to resolve?

I got the same problem in my project with pnpm.
I debugged line by line and found the problem is getAllReferences function returned a path the symlink, not the path of the real file or directory.

export function getAllReferences(importer: string, snapshot: tsTypes.IScriptSnapshot | undefined, options: tsTypes.CompilerOptions)
{
if (!snapshot)
return [];
const info = tsModule.preProcessFile(snapshot.getText(0, snapshot.getLength()), true, true);
return _.compact(_.concat(info.referencedFiles, info.importedFiles).map((reference) =>
{
const resolved = tsModule.nodeModuleNameResolver(reference.fileName, importer, options, tsModule.sys);
return resolved.resolvedModule ? resolved.resolvedModule.resolvedFileName : undefined;
}));
}

for examples,
consider this following directory

node_modules
├─.pnpm
|  ├─vue@3.2.2
|  |  ├─node_modules
|  |  |   ├─@vue
|  |  |   ├─vue
packages
|  ├─demo
|  |   ├─node_modules
|  |   │  └─vue(symlink, real path should be `project_root/node_modules/.pnpm/vue@3.2.2/node_modules/vue`)
|  |   ├─public
|  |   └─src

As you see, the real path of vue in demo should be project_root/node_modules/.pnpm/vue@3.2.2/node_modules/vue
But after resolving by tsModule.nodeModuleNameResolver in function getAllReferences , it becomes project_root/packages/demo/node_modules/vue

Vue@3.2.2 depends on @vue, which is not exist in project_root/packages/demo/node_modules, so it throw an error while resolving.

I wonder if there is any way to solve this problem.
Otherwise i can only use pnpm --shamefully-hoist to hoist all packages to root node_modules, which is similar to yarn/npm.😅

@mammadataei
Copy link

Any news?
I have the same issue

semantic error TS2305: Module '"vue"' has no exported member 'defineComponent'.

@RookieChen4

This comment was marked as spam.

@RookieChen4
Copy link

RookieChen4 commented Apr 29, 2022

@rollup/plugin-typescript solve this.

@kdoroszewicz
Copy link

rollup-plugin-ts works too and also exports compiled d.ts typings without much of a hassle.

@agilgur5 agilgur5 added the topic: monorepo / symlinks Related to monorepos and/or symlinks (Lerna, Yarn, PNPM, Rush, etc) label May 1, 2022
@agilgur5 agilgur5 added the kind: bug Something isn't working properly label May 27, 2022
@agilgur5
Copy link
Collaborator

agilgur5 commented May 27, 2022

Fixed the issue!

So I believe I've found a fix for this long-standing issue! See #332

Notes on root cause analysis / debugging

Red herring to look at resolveId hook etc

I've been looking at the code and I'm mystified. Leaving more info here for myself and other passing-by folks:

It was helpful to see this previous research, but this seemed to be a bit of a red herring... I'll explain shortly.

This issue doesn't list "plugin output with verbosity: 3" as requested in the issue template, but #330 did, and I confirmed the fix in its reproduction. I also took the reproduction here and added verbosity: 3 and confirmed it had very similar logs. I also confirmed (by locally editing rpt2 in node_modules) that the fix works for this reproduction as well.

Notably, the logs don't output anything about "resolving", so the bug here isn't in the resolveId hook at all. Which is why looking at resolve etc was a red herring.
That portion of the code also resolves directly with TS and with tsModule.sys as its host. As I found out later, ts.sys has a default implementation of realpath, which turns out to be the important missing piece here.

Error is in transform hook actually

It actually errors right after the "transpiling" log statement, which means the error is actually in the transform hook, specifically in getEmitOutput, which is implemented by TS and doesn't have host-specific code for it... But apparently it seems to rely host.realpath under-the-hood.

Implementing host.realpath fixes this

So I tried implementing host.realpath (as #332 shows), and then everything worked and the type-error disappeared!
The typings were generated correctly as well now (#330 had broken typings when using check: false).

TS docs on Compiler APIs leave a lot to be desired... 😕 😞

The TS docs are incredibly sparse when it comes to Compiler integrations such as this plugin. The Compiler API basically has two small docs/examples in the TS Wiki... and that's it. Notably, there is no mention of realpath what-so-ever in either of those.

Honestly the root cause of a lot of TS integration issues can come down to poor documentation of the Compiler API, which has been noted in lots of places.
As a result, this requires even more significant domain knowledge to figure out issues like this, as the docs aren't even helpful -- I had to literally read through the Compiler source code... and have had to do that a handful of times in the past too. That's obviously not ideal, though at least we can do something like that in open-source.

Basically you need to be a TS Compiler subject matter expert to figure out some of these things, and I'm not sure I would even call myself that despite being a regular contributor to the TS tooling ecosystem (compilers are huge after all!).

Note on Other Rollup TS plugins

@rollup/plugin-typescript solve this.

rollup-plugin-ts works too and also exports compiled d.ts typings without much of a hassle.

I'm honestly not sure how these plugins manage to get around this issue as neither of them implement host.realpath

I've mentioned this elsewhere, but why I decided to contribute here (and now I help maintain this repo) as opposed to rollup-plugin-ts is because this repo was significantly simpler to wrap my head around. rollup-plugin-ts has quite a lot of features -- which is great! But it also means that one actually needs even more domain knowledge of its structure to contribute.

@rollup/plugin-typescript historically wasn't well maintained (that's why this fork came into existence after all), but is quite a bit better maintained nowadays.

I believe all 3 plugins (including this one) have different approaches toward various problems, so unfortunately the code can differ quite a bit. And all 3 plugins have their own sets of bugs and features too 😕

Shared core tooling would be great!

I'd love for TS tooling to use more shared packages though where the approach is the same!

E.g. the host implementation and tsconfig parsing are two areas that could probably be de-duped; I'm personally working on this in my very limited free time (and notably I started contributing here during my time solo-maintaining TSDX, so already investing upstream). Shared packages and composition are especially useful for complicated and poorly documented subareas of TS (or any codebase that needs significant domain knowledge for that matter).

(one example of composition would be running rollup-plugin-dts on top of this plugin or @rollup/plugin-typescript vs. using rollup-plugin-ts's built-in custom transformer for declaration bundling. Both of these have their own issues / bugs too -- TS doing this itself would be the ideal, then the whole ecosystem benefits and can use it!)

Misc

So you are saying rpt2 should pass { preserveSymlinks: false } option to resolve?

false is actually the default of resolve, Rollup itself, TS itself, which all reflect how Node itself treats symlinks (i.e. require.resolve).
And that was a red herring anyway

I also think we may not need resolve as a dep at all since we can use built-in Node methods (resolve was originally created for browserify after all).

@agilgur5
Copy link
Collaborator

agilgur5 commented May 27, 2022

Red herring to look at resolveId hook etc

Relatedly (but different root cause), I investigated #295 more after solving this issue and stepped through the resolution logic therein.
I confirmed my suspicion that resolveId works correctly due to its direct usage of tsModule.sys in #295 (comment), where the "resolving" logs are output with the correctly resolved realpath instead of the one to the symlink.

So not just a red herring, the resolveId code actually correctly handles symlinks already.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jun 2, 2022

Released in 0.32.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly topic: monorepo / symlinks Related to monorepos and/or symlinks (Lerna, Yarn, PNPM, Rush, etc) topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs
Projects
None yet
9 participants