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

Yarn PnP support - resolveTypeReferenceDirective - "I'm calling for a resolution..." #934

Closed
johnnyreilly opened this issue May 16, 2019 · 11 comments · Fixed by #936
Closed

Comments

@johnnyreilly
Copy link
Member

It all started when @arcanis raised a PR which added Yarn PnP support to ts-loader. To quote the PR:

This PR adds a new resolveModuleName option that can be used to help TypeScript figure out the location of the files on the disk. It mimics the TypeScript API.

#862

Time passed and it was all good. Then the wonderful @arcanis submitted a very similar PR to the fork-ts-checker-webpack-plugin:

TypeStrong/fork-ts-checker-webpack-plugin#250

This PR was very similar to the ts-loader one. However, as I was reviewing it I noticed something. TypeStrong/fork-ts-checker-webpack-plugin#250 (comment)

To quote myself:

Thanks! I was just looking at the work you did on ts-loader: #862

You only added resolveModuleName there.

This adds 2 options: resolveModuleNameModule and resolveTypeReferenceDirectiveModule

Does ts-loader need a resolveTypeReferenceDirective to work with pnp as well? Or is it not necessary?

And @arcanis response:

I think that might be useful yep. When I initially made the PR for ts-loader I wasn't aware of what resolveTypeReferenceDirective was used for (/// <reference types="...">) - I just discovered it when implementing the feature for fork-ts-checker-webpack-plugin and only because create-react-app uses it to automatically define the *.svg typings.

So I decided to add the corresponding functionality to ts-loader; I gave it its own resolveTypeReferenceDirective:

#921

Now up until this point, when we created a ServicesHost in ts-loader we didn't actually supply resolveTypeReferenceDirective at all - in fact it was commented out in the codebase as you can (slightly embarrassingly) see here:

https://github.com/TypeStrong/ts-loader/pull/921/files#diff-70b0dbb6219cb6164e67ffb54a8f23c0L140

And it turned out that introducing it at all has had a breaking side effect for jest users:

#919 (comment)

Clearly this issue could be "fixed" by just dropping the resolveTypeReferenceDirectives from ts-loader. But if we do that, then I'm assuming we break Yarn PnP support too (only for type reference directives that is).

Where I am is here: I can no longer remember the reason I commented out supplying resolveTypeReferenceDirectives. Flat out, I just don't remember. And so we finally get to the question:

Is ts-loader using this API correctly? Should we be using differently? Or is it actually not to be used? I'm after guidance. Please educate me!

cc @andrewbranch

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 18, 2019

Additional info; the new option being used by ts-loader adds further issues when plugged into yarn-pnp-plugin - using my PR here: arcanis/pnp-webpack-plugin#10

The errors look like this:

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'anymatch'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'braces'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'micromatch'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'node'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'normalize-package-data'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'semver'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'tapable'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'uglify-js'.

ERROR in C:\source\ts-loader\examples\yarn-pnp\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'webpack'.

Which are similar to the errors mentioned in this issue: microsoft/TypeScript#27956

You can take the (broken) example for a test drive here: https://github.com/TypeStrong/ts-loader/tree/yarn-pnp-broken-example/examples/

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 19, 2019

Hey @andrewbranch

Thanks for all your work on this - it's been amazing. ❤️

I think there's still an issue which remains. Unfortunately my poor old laptop has just given up on life and so I'm going to try and explain this on my phone 😁 (sadly I'm now the owner of a metal brick with "Dell XPS 13" stamped on it - shakes fist at the sky / weeps / starts the 5 stages of grief)

Okay, so (just before my computer bricked) I updated the broken yarn example here: https://github.com/TypeStrong/ts-loader/tree/yarn-pnp-broken-example/examples/yarn-pnp-resolveTypeReferenceDirectives-broken to use ts-loader@6.0.1.

I figured this would resolve all the issues we'd seen. Please note this also use the PR of pnp-webpack-plugin: arcanis/pnp-webpack-plugin#10 that supplies includes resolveTypeReferenceDirective which is super minor

cc @arcanis (@andrewbranch please meet @arcanis of the Yarn team who is working hard on yarn PnP and other marvellous things, @arcanis please meet @andrewbranch of the TypeScript team 😄)

Alas, problems still remain. We're still bumping on errors like this:

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'anymatch'.

My assumption is that the 1 line PR to pnp-webpack-plugin is valid. So I'm a little baffled why these errors still present...

Is this a different issue do you think? My working assumption is that the resolveTypeReferenceDirective is necessary to enable full yarn PnP support. See conversation here: TypeStrong/fork-ts-checker-webpack-plugin#250 (comment)

For useful contrast there's also this example which is identical to the broken example apart from the package.json:

https://github.com/TypeStrong/ts-loader/tree/yarn-pnp-broken-example/examples/yarn-pnp-no-resolveTypeReferenceDirectives-working

This uses ts-loader@5.x which did not have resolveTypeReferenceDirectives in play. It works fine. (That is to say - it compiles happily) However, if code was introduced which required resolveTypeReferenceDirectives functionality behind the scenes I think we would be toast. (Technical term; much inspired by the current state of my laptop 😁)

So I don't think this "working" example is actually giving us entirely what we need.

Does all this make sense to you? I feel like I may not have done the best job explaining this... Happy to have another crack at it!

Thanks again for all the work you've been doing - it's been incredibly helpful ❤️

@johnnyreilly johnnyreilly changed the title "I'm calling for a resolution..." Yarn PnP support - resolveTypeReferenceDirective - "I'm calling for a resolution..." May 19, 2019
@johnnyreilly johnnyreilly pinned this issue May 19, 2019
@andrewbranch
Copy link
Contributor

Welp, I would love to debug and see what's happening, but the first thing I notice about PnP is—how are you supposed to debug anything? Debugging build and test tools like webpack is dependent upon being able to write something like node --inspect-brk node_modules/webpack/bin/webpack.js. In this universe, even if I go through the manual work of figuring out where webpack is, running node at that path can't figure out where all its dependencies are since you didn't start node with the custom resolver.

node --inspect-brk `which yarn` build

doesn't work either, presumably because yarn spins up a child process to actually run the npm script, so you're attaching to the wrong thing.

The other thing debugging packages relies on is linking to local—npm link or yarn link. I ran yarn link ts-loader and it gave me a success message, but .pnp.js still says it should pull ts-loader from some library cache. Does PnP just not work with yarn link?

Without an active debug session, there’s not much I can do—punting to @arcanis

@arcanis
Copy link
Contributor

arcanis commented May 19, 2019

Hey! I'll give a better look at this issue soon but I figured I should first quickly answer your question, @andrewbranch 😃

how are you supposed to debug anything?

A few things:

  • The PnP hook can be manually injected with --require <path/to/.pnp.js>.
  • But it also can be automatically injected via yarn node
  • You can instruct Yarn to materialize any package with yarn unplug <name>

So in your case it would be something like this:

$> yarn unplug webpack
$> yarn node --inspect-brk $(yarn -s bin webpack)

The first line can be skipped if you don't intend to edit webpack's source code (you don't need to materialize the package).

@arcanis
Copy link
Contributor

arcanis commented May 19, 2019

I'm trying to run the example locally but it seems to succeed - what did I do wrong? I did the following:

  • git clone git@github.com:TypeStrong/ts-loader
  • git checkout yarn-pnp-broken-example
  • cd examples/yarn-pnp-resolveTypeReferenceDirectives-broken
  • yarn install
  • yarn build

I'm using Yarn 1.16 and got the following output:

yarn run v1.16.0
warning package.json: No license field
$ webpack --mode production
Hash: 73451fa1973643a857c1
Version: webpack 4.20.2
Time: 1157ms
Built at: 05/19/2019 6:59:57 PM
  Asset      Size  Chunks             Chunk Names
main.js  7.44 KiB       0  [emitted]  main
Entrypoint main = main.js
[0] ./src/index.ts 85 bytes {0} [built]
[1] ./src/render.ts 138 bytes {0} [built]
Done in 3.33s.

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 19, 2019

Weird - that's not what I get / got.... Let me retest now....

Yup - I get this:

C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken [yarn-pnp-broken-example ≡ +0 ~2 -0 !]> yarn
yarn install v1.16.0
warning package.json: No license field
warning vanilla@1.0.0: No license field
[1/5] Resolving packages...
[2/5] Fetching packages...
info fsevents@1.2.4: The platform "win32" is incompatible with this module.
info "fsevents@1.2.4" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/5] Linking dependencies...
[5/5] Building fresh packages...

success Saved lockfile.
Done in 3.54s.
C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken [yarn-pnp-broken-example ≡ +0 ~1 -0 !]> yarn build
yarn run v1.16.0
warning package.json: No license field
$ webpack --mode production
Hash: be4c4d357a59eb6dbd46
Version: webpack 4.20.2
Time: 1531ms
Built at: 05/19/2019 7:12:23 PM
 1 asset
Entrypoint main = main.js
[0] ./src/index.ts 89 bytes {0} [built]
[1] ./src/render.ts 143 bytes {0} [built]

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'anymatch'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'braces'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'micromatch'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'node'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'normalize-package-data'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'semver'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'tapable'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'uglify-js'.

ERROR in C:\source\ts-loader\examples\yarn-pnp-resolveTypeReferenceDirectives-broken\tsconfig.json
[tsl] ERROR
      TS2688: Cannot find type definition file for 'webpack'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I'm Windows BTW - I don't know if that is a relevant factor...

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 19, 2019

Okay this is super weird.... If I reclone my own repo everything works:

git clone https://github.com/TypeStrong/ts-loader.git
cd ts-loader
git checkout yarn-pnp-broken-example
cd examples/yarn-pnp-resolveTypeReferenceDirectives-broken
yarn install
yarn build

The above reliably works...

My local copy is theoretically identical (I mean Git doesn't lie does it?!) and yet it doesn't work.... Not sure why that would be?

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 19, 2019

Okay so I think there isn't actually a problem with ts-loader or with the PR to pnp-webpack-plugin either. I'm going to close this issue. Thanks @arcanis / @andrewbranch for looking into this. I super appreciate it.

@arcanis in case you're curious, I ran BeyondCompare over the working / broken versions and got this:

Screenshot 2019-05-19 20 02 04

On the left above you have the working version. On the right you have the original (and broken) version. The differences seem to reside only in the node_modules/cache folder. I have no idea what that means. Does this mean anything to you? Thought I'd share it in case I'd stumbled upon a giant gotcha for yarn PnP 😄

@arcanis
Copy link
Contributor

arcanis commented May 19, 2019

Hmm I really don't know 🤔 I know that node_modules/.cache is typically used by Babel, but since it's not used in this example ... maybe TS also uses it and didn't see some file had changed?

@johnnyreilly
Copy link
Member Author

johnnyreilly commented May 19, 2019

It's weird... I blew away .pnp.js and node_modules, yarn and yarn build. node_modules reappears in the yarn build.... Same error. Dunno. Can't explain it. Given it's under uglifyjs I'm imagining it's webpack related.

Maybe it's nothing to be worried about. If it's something significant I'm sure it'll come up again in a more obvious way in future 😄

Side note: when I was experimenting with using file paths in the package.json I got really strange results when I did a local path like this:

"ts-loader": "file:../../../"

It worked but it took about 100 seconds to perform a yarn install. Peculiar.

@andrewbranch
Copy link
Contributor

  • The PnP hook can be manually injected with --require <path/to/.pnp.js>.
  • But it also can be automatically injected via yarn node
  • You can instruct Yarn to materialize any package with yarn unplug

Amazing, I was hoping there would be some magic I was unaware of like that. Thanks @arcanis!

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

Successfully merging a pull request may close this issue.

3 participants