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

resolveTypeReferenceDirective support for yarn PnP #921

Merged
merged 4 commits into from
Apr 22, 2019

Conversation

johnnyreilly
Copy link
Member

@johnnyreilly johnnyreilly commented Apr 21, 2019

This PR should (when completed) add resolveTypeReferenceDirective support to ts-loader to complement the existing resolveModuleName added by @arcanis in #862 which was added to support yarn PnP.

Other inspiration: TypeStrong/fork-ts-checker-webpack-plugin#250

I've a number of questions which I'm hoping people can advise on. I'd like to use this PR to go through these as I think the answers to them will be a generally useful resource in future.

Heads up: I have never used PnP before, but I'm excited the possibilities it may provide. I think it's somewhat useful that I don't know tons about it as the questions I ask will likely be the sorts of things that others wonder about too.

I hope this is cool with you @arcanis?

So this PR should be a combination of discussion and then implementation informed by the discussion.

@johnnyreilly johnnyreilly changed the title prepare the ground for type directives resolveTypeReferenceDirective support for yarn PnP Apr 21, 2019
@arcanis
Copy link
Contributor

arcanis commented Apr 21, 2019

Awesome 👍

@johnnyreilly
Copy link
Member Author

johnnyreilly commented Apr 21, 2019

First question: pnp-webpack-plugin vs ts-pnp

The PnP docs point you here: https://github.com/arcanis/pnp-webpack-plugin#ts-loader-integration

Which suggests you should use the pnp-webpack-plugin to marry ts-loader and PnP:

const PnpWebpackPlugin = require(`pnp-webpack-plugin`);

module.exports = {
  module: {
    rules: [{
      test: /\.ts$/,
      loader: require.resolve('ts-loader'),
      options: PnpWebpackPlugin.tsLoaderOptions(),
    }],
  },
};

In fact ts-pnp itself suggests:

Note that ts-pnp is a low-level package - you shouldn't have to use it unless you're writing a TS compiler host. If you just want to write TypeScript and have it Just Work™, take a look at pnp-webpack-plugin instead.

So it looks like we should be using pnp-webpack-plugin and not ts-pnp.

That being the case, It looks like a tweak may be required here:

https://github.com/arcanis/pnp-webpack-plugin/blob/b09fbdc2a9f16dc3837454b8d367963b1a30655f/index.js#L140

module.exports.tsLoaderOptions = (options = {}) => pnp ? Object.assign({}, options, {
  resolveModuleName: require('ts-pnp').resolveModuleName,
}) : options;

To add in resolveTypeReferenceDirective to this. (Once it exists in ts-loader)

Does that sound right?

Also, I'm guessing using tsLoaderOptions is mandatory if you want to use ts-loader with PnP as it's this that supplies resolveModuleName to ts-loader.

Otherwise you would have to manually supply this option to ts-loader like so:

  resolveModuleName: require('ts-pnp').resolveModuleName,

Do I have this about right?

@arcanis
Copy link
Contributor

arcanis commented Apr 21, 2019

Does that sound right?

Yep - although my warning in ts-pnp is maybe a bit too strong in that as you can see there isn't much boilerplate needed to integrate ts-pnp with ts-loader without going through pnp-webpack-plugin (ts-pnp isn't a "private api" and will respect semver, so it's safe to use it directly).

The wording is mostly intended for average users, which don't really care much about the setup of TS and PnP and just want something that work out of the box. In their case they can just use pnp-webpack-plugin which will take care of this through a simplified API.

@johnnyreilly
Copy link
Member Author

Hey @arcanis,

I've implemented what I think makes sense based on our conversation. Do you want to take a quick glance and let me know if I'm headed roughly in the right direction? There's a bit of noise in the servicesHost.ts module but hopefully otherwise it makes sense...

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me from an implem standpoint!

@@ -137,30 +153,81 @@ export function makeServicesHost(
trace: log.log,
log: log.log,

/* Unclear if this is useful
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eheh it was 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, right? How could I imagine a comment like that wouldn't come back to bite me 😄

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 this pull request may close these issues.

2 participants