Implements a custom resolveModuleName option #862

arcanis · 2018-10-26T23:22:29Z

Solves #861 - 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.

I published a first implementation as part of ts-pnp, and it can be tested using the pnp-webpack-plugin which provides an integration.

I'm not too sure how and where I should test this - any advice?

johnnyreilly · 2018-10-27T05:35:51Z

src/interfaces.ts

+export type CustomResolveModuleName = (
+  moduleName: string,
+  containingFile: string,
+  options: typescript.CompilerOptions,


Could this be called compilerOptions also please?

johnnyreilly · 2018-10-27T05:43:00Z

src/servicesHost.ts

-    compilerOptions,
-    moduleResolutionHost
-  );
+  const tsResolver = (moduleName: string, containingFile: string, compilerOptions: typescript.CompilerOptions, moduleResolutionHost: typescript.ModuleResolutionHost) =>


Given tsResolver is a pure function could we break this out in to a separate function please? That saves creating a new function each time this runs.

johnnyreilly · 2018-10-27T05:46:04Z

Thanks for this! Could you address the comments and fix the linting errors that are showing up in CI please?

johnnyreilly · 2018-10-27T05:49:49Z

Regarding testing, it would be good to implement an execution test to cover this. You can find out more about these here: https://github.com/TypeStrong/ts-loader/blob/master/test/execution-tests/README.md

A good example to clone as a basis for your test might be this one:

https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests/3.0.1_projectReferences

or

https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests/3.0.1_resolveJsonModule

johnnyreilly · 2018-10-27T18:06:06Z

src/servicesHost.ts

  const tsResolution = customResolveModuleName
-    ? customResolveModuleName(moduleName, containingFile, compilerOptions, moduleResolutionHost, tsResolver)
-    : tsResolver(moduleName, containingFile, compilerOptions, moduleResolutionHost);
+    ? customResolveModuleName(moduleName, containingFile, compilerOptions, moduleResolutionHost, applyTsResolver.bind(null, compiler))


I hadn't expected the .bind(null, compiler) would be necessary... I'm guessing it is? What happens without it? Presumably some kind of this issue?

No, we simply need the compiler variable to be able to call compiler.resolveModuleName (and compiler is obtained from instance, which isn't a global)

johnnyreilly · 2018-10-27T18:08:52Z

test/execution-tests/3.0.1_resolveModuleName/tsconfig.json

+{
+	"compilerOptions": {
+		"noEmitOnError": true,
+		"resolveJsonModule": true


can we remove the "resolveJsonModule": true please? It doesn't do any harm but it might lead someone who was reading the test to believe that it was necessary for resolveModuleName to work. (Not least because of the similarity in naming 😄 )

johnnyreilly · 2018-10-27T18:09:24Z

test/execution-tests/3.0.1_resolveModuleName/test/app.tests.ts

+import * as app from '../src/app';
+
+// We don't actually care about the result of the following operations;
+// what we care about is typescript resolving json as modules


Could you amend the comment to reflect what the intention of the test is please?

I left the comment because it's actually the same intent - we don't care about what is being built, just about TS being able to make the resolution

Oh, I see a "json" is in the comment - will change it 👍

johnnyreilly · 2018-10-27T18:17:39Z

Good work on the changes @arcanis! I've a couple more comments that I've added just now.

Well done on the execution test. Looks good! Question though: it looks like it's only resolving types from foo-pkg / bar-pkg. There doesn't seem to be any actual JS being resolved. It might be good to include some JS as well so we know that as well as the compiler resolving types correctly we can see the actual code that we want to run coming through as well.

Does that sound sensible? Or have I misunderstood your intent?

arcanis · 2018-10-27T18:22:15Z

@johnnyreilly I wasn't entirely sure if the generated code had to be tested as well, since I wasn't sure whether ts-loader was entirely relying on TypeScript for the resolution or not (versus only using it for the type resolution, leaving the regular Webpack resolver setup the links between the packages). What do you think?

johnnyreilly · 2018-10-27T18:39:49Z

@johnnyreilly I wasn't entirely sure if the generated code had to be tested as well,

Yes please!

since I wasn't sure whether ts-loader was entirely relying on TypeScript for the resolution or not (versus only using it for the type resolution, leaving the regular Webpack resolver setup the links between the packages).

It's a good question. ts-loader uses webpack for actual file resolution:

https://github.com/TypeStrong/ts-loader/blob/master/src/resolver.ts

Funnily enough, it's on my radar to try out using TypeScript for resolve module names resolution too, but as yet I haven't.

See: #757 (comment)

arcanis · 2018-10-27T20:22:43Z

Let me know if that matches what you had in mind :)

johnnyreilly · 2018-10-28T06:38:30Z

src/servicesHost.ts

+          containingFile,
+          compilerOptions,
+          moduleResolutionHost,
+          applyTsResolver.bind(null, compiler)


Seeing a .bind my perf spider sense is tingling!

https://stackoverflow.com/questions/42117911/lambda-functions-vs-bind-memory-and-performance

I wonder if we could switch this to a lamba instead? It'll be slightly more verbose but hopefully should perform better...

johnnyreilly · 2018-10-28T06:40:45Z

test/execution-tests/3.0.1_resolveModuleName/test/app.tests.ts

@@ -0,0 +1,13 @@
+import * as app from '../src/app';
+
+// We don't actually care about the result of the following operations;


This is not true actually; we do care about the result of the following operations! Can we amend the comment to make test intent clearer please? We care both about type resolution and that the code executes...

johnnyreilly · 2018-10-28T06:53:38Z

Thanks chap! I've put a few comments in and I've also fixed the failing comparison test on your branch.

I think the execution test is better but it's still not quite what I had in mind; let me explain:

As I understand it foo-pkg etc represent node modules? i.e. that which you would expect to find in the node_modules folder.

That being the case you'd likely not consume them with raw TypeScript files in; you'd either:

Consume a node module with just JS and rely on external type definitions (whether defined by yourself or via an @types/ scoped package published to npm by Definitely Typed.

Or

Consume a node module with JS and .d.ts definition files in the package too. (Eg mobx)

If my understanding is correct then ideally our tests should be covering those scenarios rather than exporting raw .ts.

It's possible my understanding of this feature is not quite right; feel free to set me straight! 😄

arcanis · 2018-10-28T23:42:08Z

I think the first scenario you mention isn't relevant to ts-loader - it's the resolveModuleName implementation that handles the fact that @types is tried before other possible options (check my implementation).

I've updated baz-pkg to be the case you describe (a index.js + a index.d.ts), let me know if that works for you 🙂

johnnyreilly · 2018-10-29T06:07:51Z

Cool - thanks for that! I've just fixed up the linting errors; hopefully we should now see a clean CI 😄

Question: in the test you only make use of the moduleName and parentResolver parameter; the others are all pass through parameters:

ts-loader/test/execution-tests/3.0.1_resolveModuleName/webpack.config.js

Lines 16 to 23 in d8f3735

    
           resolveModuleName: (moduleName, containingFile, compilerOptions, compilerHost, parentResolver) => { 
        
               switch (moduleName) { 
        
                   case 'foo': return parentResolver(path.join(__dirname, 'foo-pkg'), containingFile, compilerOptions, compilerHost); 
        
                   case 'bar': return parentResolver(path.join(__dirname, 'bar-pkg'), containingFile, compilerOptions, compilerHost); 
        
                   case 'baz': return parentResolver(path.join(__dirname, 'baz-pkg'), containingFile, compilerOptions, compilerHost); 
        
                   default: return parentResolver(moduleName, containingFile, compilerOptions, compilerHost); 
        
               } 
        
           },

That being the case, does it make sense for the resolveModuleName function to have all those parameters? I'm assuming it does; but I wanted to double check they weren't being passed on for no reason 😄

All things being equal, I suspect we're pretty much there!

johnnyreilly · 2018-10-30T08:14:26Z

Ping @arcanis 😉

arcanis · 2018-10-30T10:40:00Z

Sorry, was flying yesterday 🙂

So it don't use those values yet (and I'm not too familiar with TS to know what they're used for, tbh), but since the TS resolveModuleName hook requires all four of them I figured it'd make sense to also accept them, since they're easily available to us while the hook doesn't have any way to know them otherwise l.

One particular advantage is that it will make it easier for different projects/loaders to use the same custom resolveModuleName implementation (which would be a bit more difficult if they were each expecting a different set of parameters).

johnnyreilly · 2018-10-30T10:49:10Z

That sounds reasonable! Do you have anymore work to do on this or is this ready to merge from your perspective?

arcanis · 2018-10-30T11:30:01Z

Nope, I think it's ready on my side!

johnnyreilly · 2018-10-30T11:56:29Z

Nice! Thanks for your work!

Would you be able to update the package.json and the CHANGELOG.md (don't forget to thank yourself!)?

That'll help me as I look to merge / publish this.

arcanis · 2018-10-30T20:26:48Z

Here you go! Minor, right? 👍

johnnyreilly · 2018-10-31T06:24:26Z

Thanks! Published with v5.3.0

Implements a custom resolveModuleName option

6367073

arcanis mentioned this pull request Oct 26, 2018

Expose resolveModuleNames #861

Closed

johnnyreilly reviewed Oct 27, 2018

View reviewed changes

arcanis added 2 commits October 27, 2018 12:57

Addresses feedback

46d721a

Adds a test

f2f6026

johnnyreilly reviewed Oct 27, 2018

View reviewed changes

Adresses feedback

289d904

Expands the test scope

9a0540a

johnnyreilly added 3 commits October 28, 2018 06:17

Fix validateLoaderOptionNames test

0bc78bd

prettier formatting

8b694a8

Be explicit about the possible value of customResolveModuleName

afa570d

johnnyreilly reviewed Oct 28, 2018

View reviewed changes

Feedback round 3

ad4ed7b

Fix lint issues

d8f3735

Bumps package.json, updates changelog

bee1c37

arcanis force-pushed the custom-resolve-module-name branch from d351782 to bee1c37 Compare October 30, 2018 20:26

johnnyreilly merged commit 08cd5a3 into TypeStrong:master Oct 31, 2018

arcanis mentioned this pull request Oct 31, 2018

Custom resolveModuleName TypeStrong/fork-ts-checker-webpack-plugin#181

Closed

arcanis mentioned this pull request Nov 9, 2018

Add new moduleResolution option: yarn-pnp microsoft/TypeScript#28289

Open

4 tasks

arcanis mentioned this pull request Dec 13, 2018

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

Open

This was referenced Apr 20, 2019

Adds support for custom resolvers to CompilerHost TypeStrong/fork-ts-checker-webpack-plugin#250

Merged

resolveTypeReferenceDirective support for yarn PnP #921

Merged

johnnyreilly mentioned this pull request May 16, 2019

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

Closed

snyk-bot mentioned this pull request Mar 18, 2020

[Snyk] Upgrade ts-loader from 5.3.0 to 5.4.5 retorquere/zotero-storage-scanner#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements a custom resolveModuleName option #862

Implements a custom resolveModuleName option #862

arcanis commented Oct 26, 2018

johnnyreilly Oct 27, 2018

johnnyreilly Oct 27, 2018

johnnyreilly commented Oct 27, 2018

johnnyreilly commented Oct 27, 2018 •

edited

Loading

johnnyreilly Oct 27, 2018

arcanis Oct 27, 2018

johnnyreilly Oct 27, 2018

arcanis Oct 27, 2018

johnnyreilly Oct 27, 2018

arcanis Oct 27, 2018

arcanis Oct 27, 2018

johnnyreilly commented Oct 27, 2018 •

edited

Loading

arcanis commented Oct 27, 2018

johnnyreilly commented Oct 27, 2018 •

edited

Loading

arcanis commented Oct 27, 2018

johnnyreilly Oct 28, 2018

johnnyreilly Oct 28, 2018

johnnyreilly commented Oct 28, 2018 •

edited

Loading

arcanis commented Oct 28, 2018 •

edited

Loading

johnnyreilly commented Oct 29, 2018 •

edited

Loading

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018 •

edited

Loading

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018

johnnyreilly commented Oct 31, 2018

		@@ -0,0 +1,13 @@
		import * as app from '../src/app';

		// We don't actually care about the result of the following operations;

Implements a custom resolveModuleName option #862

Implements a custom resolveModuleName option #862

Conversation

arcanis commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyreilly commented Oct 27, 2018

johnnyreilly commented Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyreilly commented Oct 27, 2018 • edited Loading

arcanis commented Oct 27, 2018

johnnyreilly commented Oct 27, 2018 • edited Loading

arcanis commented Oct 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyreilly commented Oct 28, 2018 • edited Loading

arcanis commented Oct 28, 2018 • edited Loading

johnnyreilly commented Oct 29, 2018 • edited Loading

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018 • edited Loading

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018

johnnyreilly commented Oct 30, 2018

arcanis commented Oct 30, 2018

johnnyreilly commented Oct 31, 2018

johnnyreilly commented Oct 27, 2018 •

edited

Loading

johnnyreilly commented Oct 27, 2018 •

edited

Loading

johnnyreilly commented Oct 27, 2018 •

edited

Loading

johnnyreilly commented Oct 28, 2018 •

edited

Loading

arcanis commented Oct 28, 2018 •

edited

Loading

johnnyreilly commented Oct 29, 2018 •

edited

Loading

arcanis commented Oct 30, 2018 •

edited

Loading