Adds support for custom resolvers to CompilerHost #250

arcanis · 2019-04-18T18:09:20Z

Work in progress to add support for custom resolvers to fork-ts-checker-webpack-plugin (and thus create-react-app!). Closes #181.

The plumbing within CompilerHost is good but I'm not sure what's the best way to send the custom resolvers to the worker, since functions cannot cross the boundaries of Node processes. From what I saw the configuration is usually forwarded through the environment, so I guess it would require two new environment variable that would contain the path to modules that would export the resolvers, which would then be dynamically required in the worker thread - does that look coherent?

Of note, I think I should theoretically duplicate the code for both ApiIncrementalChecker and IncrementalChecker, but given that CRA seems to use the first one I'd prefer to focus my effort on it unless there's technical reasons (like ApiIncrementalChecker being deprecated or something).

cc @johnnyreilly

johnnyreilly · 2019-04-18T18:56:05Z

First of all thank you for the work - I really appreciate it!

I'd love to get this functionality into the plugin.

The plumbing within CompilerHost is good but I'm not sure what's the best way to send the custom resolvers to the worker, since functions cannot cross the boundaries of Node processes. From what I saw the configuration is usually forwarded through the environment, so I guess it would require two new environment variable that would contain the path to modules that would export the resolvers, which would then be dynamically required in the worker thread - does that look coherent?

I think you're suggesting a similar approach to what we did in ts-loader here: TypeStrong/ts-loader#745

Is that about the same thing? And yeah - I think it's pretty much the only game in town when passing functionality from process to process in node. It doesn't feel tremendous. Maybe you're aware of any other alternatives? Always open to learning new possibilities! If not then I guess we can do no other.

Of note, I think I should theoretically duplicate the code for both ApiIncrementalChecker and IncrementalChecker, but given that CRA seems to use the first one I'd prefer to focus my effort on it unless there's technical reasons (like ApiIncrementalChecker being deprecated or something).

Get the ApiIncrementalChecker working first I think. We will need both to work before we merge as Vue doesn't work with it yet. (Although @phryneas may change that #231 😊)

arcanis · 2019-04-18T20:42:39Z

Here we go! This diff plugs two new options into the plugin:

resolveModuleNameModule
resolveTypeReferenceDirectiveModule

Those options are expected to be the path to a file that will be required and whose symbols will be extracted (since we use different symbol names it makes it possible to export both in the same file).

I tested it with create-react-app by manually modifying the Webpack configuration to inject those options, and it seemed to work out of the box with the following resolver:

const {resolveModuleName} = require(`ts-pnp`);

exports.resolveModuleName = (typescript, moduleName, containingFile, compilerOptions, resolutionHost) => {
  return resolveModuleName(moduleName, containingFile, compilerOptions, resolutionHost, typescript.resolveModuleName);
};

exports.resolveTypeReferenceDirective = (typescript, moduleName, containingFile, compilerOptions, resolutionHost) => {
  return resolveModuleName(moduleName, containingFile, compilerOptions, resolutionHost, typescript.resolveTypeReferenceDirective);
};

phryneas · 2019-04-18T21:26:59Z

Please take also a look at #231, where I am in the progress of moving the resolveModuleName logic out ouf IncrementalChecker and ApiIncrementalChecker to one shared method in https://github.com/Realytics/fork-ts-checker-webpack-plugin/pull/231/files#diff-c91a0cb3fbe6ce4c93ee9d3d2807e353R29

Also, I've done some experimentation with "plugging some code that is available in the forked process" in #227 and like you, I came to the conclusion that passing the name of a module/file to be included is probably the way to go.

(In the end, #231 in combination with something like #227 is essentially a blueprint to add other file extensions and compilers to fork-ts-checker-webpack-plugin so I'd really like your feedback there, too :) )

arcanis · 2019-04-18T22:06:14Z

Oh nice I didn't see your work, I'll give it a look next week! A cursory glance makes it look like a large in-depth refactoring; would you say #231 is a blocker to land #250? I'm trying to get a sense of what would be the best path to ship the feature before the cra team releases their 3.0 😃

phryneas · 2019-04-18T22:17:35Z

Might depend a bit on how you fast you need this merged - I wouldn't be against adding this and re-integrating it in #231 in parallel if you need this very fast.

But we definitely should coordinate configuration options you add with this, so we don't get any weird conflicts there.

johnnyreilly · 2019-04-19T05:37:07Z

I wouldn't be against adding this and re-integrating it in #231 in parallel if you need this very fast.

I agree given that the changes in this PR are much more restricted than #231 then it will likely make sense for this to go in first.

But we definitely should coordinate configuration options you add with this, so we don't get any weird conflicts there.

When you get a moment @phryneas could you take a look at this PR and call out if there's anything that you see as a potential problem?

johnnyreilly · 2019-04-19T05:43:50Z

I think this looks good @arcanis. Could you provide some documentation on how this should be used to the README.md please? I'm guessing your comment here: #250 (comment) might get you partway there

Could you also get this plugged (ha!) in to the IncrementalChecker too.

Tests. Some kind of integration tests would be 👍👍.

arcanis · 2019-04-19T11:29:58Z

Here you go!

phryneas · 2019-04-19T13:53:12Z

I took a look at it - I'll have to re-implement most of it for #231, but the options look okay and with the integration test I have something to test again, so no complaints from me 😁

phryneas · 2019-04-19T15:22:36Z

oh, one nitpick - before this makes it into a changelog - could you change the PR title from "Adds support for custom compilers to CompilerHost " to "Adds support for custom resolvers to CompilerHost"? :)

johnnyreilly · 2019-04-19T19:38:02Z

Can you catch up your branch with master please @arcanis?

arcanis · 2019-04-19T20:21:40Z

Seems like there's a problem on Node 8 🤔

arcanis · 2019-04-19T20:32:53Z

Should be fixed - apparently Node 8 stringifies undefined values in the env object before spawning the process (whereas Node 10 removes them). I never noticed this before 🤨

johnnyreilly

I think we're pretty much ready to go. I'd rather we lost the any but honestly that's of minor importance despite my nervous twitches 😏

Would you like to update the package.json with a new version and CHANGELOG.md please? This is a new feature and I think it's non breaking?

src/index.ts

johnnyreilly · 2019-04-20T05:52:55Z

Thanks! I was just looking at the work you did on ts-loader: TypeStrong/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?

arcanis · 2019-04-20T05:57:10Z

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.

johnnyreilly · 2019-04-20T06:28:53Z

Cool - I might have a crack at that myself. It'll give me an excuse to get my hands dirty with pnp. I'd like to add that and put together an example repo which illustrates how you can use ts-loader with pnp. Is it okay if I ping you with questions when I'm doing that?

Is there anything else required for this PR? If not then shall we merge and 🛳️?

arcanis · 2019-04-20T06:32:54Z

Is it okay if I ping you with questions when I'm doing that?

Of course! Feel free to pop onto our Discord channel and mention me there 👍

Is there anything else required for this PR? If not then shall we merge and 🛳️?

johnnyreilly · 2019-04-20T07:56:45Z

Will ship with https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.1.0 (out as soon as Travis gets done here: https://travis-ci.org/Realytics/fork-ts-checker-webpack-plugin/builds/522420897

johnnyreilly · 2019-04-22T04:39:21Z

There's a possibility that this PR may have introduced a regression. See: #258

johnnyreilly · 2019-04-22T04:52:36Z

Hey @arcanis

Bad news - it looks like there is a regression 😢

The details are in #258 including simple repro repo - it looks like we don't have a test that covers this case which is a shame and definitely something I'll remedy in future.

The issue is, interestingly around paths resolution in TypeScript in tsconfig.json. I'm pretty sure you already know plenty about this given your work on PnP 😄. I've written briefly on the topic here: https://blog.johnnyreilly.com/2018/08/killing-relative-paths-with-typescript-and.html just to give some use case examples.

Is there any chance you'd be able to look into this? If you don't have time (perfectly understandable) I might have to look at reverting this PR for now so users aren't impacted.

johnnyreilly · 2019-04-22T05:31:17Z

src/index.ts

+      CONTEXT: this.compiler.options.context,
+      TSLINTAUTOFIX: String(this.tslintAutoFix),
+      WATCH: this.isWatching ? this.watchPaths.join('|') : '',
+      WORK_DIVISION: String(Math.max(1, this.workersNumber)),


I don't know if this is an issue or not but I just noticed that the options now generally use String(....) for values. So what we're previously number or boolean will now be string....

Possible issue?

… manually after merging Revert "Enables custom resolver only if they're used in incremental mode (TypeStrong#260)" This reverts commit 3990a13. Revert "Adds support for custom resolvers to CompilerHost (TypeStrong#250) - fixes TypeStrong#181" This reverts commit dfd8933.

This reverts commit 9db85d7.

Adds support for custom compilers to CompilerHost

cf71864

Attaches the options to the plugin

ae4a6d1

arcanis force-pushed the custom-resolvers branch from a3bc14f to ae4a6d1 Compare April 18, 2019 20:39

arcanis mentioned this pull request Apr 18, 2019

yarn pnp doesn't work with typescript facebook/create-react-app#5647

Open

Maël Nison added 3 commits April 19, 2019 11:59

Adds support for IncrementalChecker

58e86bf

Adds a test

311534f

Updates the README

63ecccf

arcanis force-pushed the custom-resolvers branch from 86f4dcc to 63ecccf Compare April 19, 2019 11:28

Merge branch 'master' into custom-resolvers

613b808

arcanis changed the title ~~Adds support for custom compilers to CompilerHost~~ Adds support for custom resolvers to CompilerHost Apr 19, 2019

Merge branch 'master' into custom-resolvers

70f348f

Fixes undefined env values on Node 8

9536006

johnnyreilly reviewed Apr 20, 2019

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Maël Nison added 2 commits April 20, 2019 06:34

Bumps version

d03b29b

Avoids any

4d06c61

johnnyreilly approved these changes Apr 20, 2019

View reviewed changes

johnnyreilly merged commit dfd8933 into TypeStrong:master Apr 20, 2019

arcanis mentioned this pull request Apr 20, 2019

Adds the configuration for PnP/Typescript facebook/create-react-app#6856

Merged

phryneas mentioned this pull request Apr 20, 2019

fix(tests): rework vue integration tests #253

Merged

johnnyreilly mentioned this pull request Apr 21, 2019

resolveTypeReferenceDirective support for yarn PnP TypeStrong/ts-loader#921

Merged

johnnyreilly mentioned this pull request Apr 22, 2019

tsconfig paths resolution not working in v.1.1.0 #258

Closed

johnnyreilly reviewed Apr 22, 2019

View reviewed changes

phryneas added a commit to phryneas/fork-ts-checker-webpack-plugin that referenced this pull request Apr 27, 2019

manually re-apply commits for TypeStrong#250 and TypeStrong#260

928b016

This reverts commit 9db85d7.

johnnyreilly mentioned this pull request May 16, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for custom resolvers to CompilerHost #250

Adds support for custom resolvers to CompilerHost #250

arcanis commented Apr 18, 2019

johnnyreilly commented Apr 18, 2019 •

edited

Loading

arcanis commented Apr 18, 2019

phryneas commented Apr 18, 2019 •

edited

Loading

arcanis commented Apr 18, 2019

phryneas commented Apr 18, 2019

johnnyreilly commented Apr 19, 2019

johnnyreilly commented Apr 19, 2019 •

edited

Loading

arcanis commented Apr 19, 2019

phryneas commented Apr 19, 2019

phryneas commented Apr 19, 2019

johnnyreilly commented Apr 19, 2019

arcanis commented Apr 19, 2019

arcanis commented Apr 19, 2019

johnnyreilly left a comment

johnnyreilly commented Apr 20, 2019

arcanis commented Apr 20, 2019

johnnyreilly commented Apr 20, 2019

arcanis commented Apr 20, 2019

johnnyreilly commented Apr 20, 2019

johnnyreilly commented Apr 22, 2019

johnnyreilly commented Apr 22, 2019 •

edited

Loading

johnnyreilly Apr 22, 2019

Adds support for custom resolvers to CompilerHost #250

Adds support for custom resolvers to CompilerHost #250

Conversation

arcanis commented Apr 18, 2019

johnnyreilly commented Apr 18, 2019 • edited Loading

arcanis commented Apr 18, 2019

phryneas commented Apr 18, 2019 • edited Loading

arcanis commented Apr 18, 2019

phryneas commented Apr 18, 2019

johnnyreilly commented Apr 19, 2019

johnnyreilly commented Apr 19, 2019 • edited Loading

arcanis commented Apr 19, 2019

phryneas commented Apr 19, 2019

phryneas commented Apr 19, 2019

johnnyreilly commented Apr 19, 2019

arcanis commented Apr 19, 2019

arcanis commented Apr 19, 2019

johnnyreilly left a comment

Choose a reason for hiding this comment

johnnyreilly commented Apr 20, 2019

arcanis commented Apr 20, 2019

johnnyreilly commented Apr 20, 2019

arcanis commented Apr 20, 2019

johnnyreilly commented Apr 20, 2019

johnnyreilly commented Apr 22, 2019

johnnyreilly commented Apr 22, 2019 • edited Loading

johnnyreilly Apr 22, 2019

Choose a reason for hiding this comment

johnnyreilly commented Apr 18, 2019 •

edited

Loading

phryneas commented Apr 18, 2019 •

edited

Loading

johnnyreilly commented Apr 19, 2019 •

edited

Loading

johnnyreilly commented Apr 22, 2019 •

edited

Loading