Goal: add option to overwrite existing modules

[asasvirtuais]

My goal was to import preact like this:

import { render, h } from 'preact';

But have my JS code like this:

import { render, h } from "https://cdnjs.cloudflare.com/ajax/libs/preact/10.5.7/preact.module.min.js";

But instead I was getting

import { render, h } from 'preact';

So I looked at the code here and made a few changes that solved my problem, I am not sure if this breaks anything else, havent tested it, but it worked to solve my problem, so here it is.

Documentation on the feature/option:

Option: overwriteNodeModules

{
	"transform": "typescript-transform-paths",
	"overwriteNodeModules" : true
}

If you wish to replace an import for a package that exists on node_modules you can set the option "overwriteNodeModules".

This can be useful when you are planning to use a cdn like in this example:

import { render, h } from 'preact';

Becomes:

import { render, h } from "https://cdnjs.cloudflare.com/ajax/libs/preact/10.5.7/preact.module.min.js";

[nonara]

Hi Icaro! Thanks for the PR. Can you show me the paths config you're using?

[asasvirtuais]

Sure

"paths": {
	"preact/hooks": ["https://cdnjs.cloudflare.com/ajax/libs/preact/10.5.7/hooks.module.min.js"],
	"preact": ["https://cdnjs.cloudflare.com/ajax/libs/preact/10.5.7/preact.module.min.js"]
}

Anything else?

[nonara]

@danielpza In the same vein, at least!

For that, I'm planning on adding include and exclude as config options. Include would operate as a strict whitelist pattern, where exclude would selectively ignore. Only one could be specified.

In addition, I was going to introduce @transform-path <custom-path> and @no-transform-path as statement level JSDoc tags to custom transform or bypass transformation for specific paths.

[nonara]

As far as this goes, I think Icaro's approach makes sense. Our default behaviour is to not transform for any existing node modules, which in retrospect, I'm not entirely positive should be the default. But to avoid breaking side-effects, I think introducing a config option is a good idea, and I'm good with the option name.

@danielpza What do you think?

[danielpza]

PR looks good, I think the only issue is I find the name a little confusing. Is it only intended for node modules? Looking at the code perhaps a more appropriate name would be allowExternalLibraryImports or something like that.

[nonara]

@danielpza I wasn't sure on that either. Generally my approach on this sort of thing is to weigh the idiomatic popularity and go with what would be more widely understood unless one of the the specific nomenclatures is more precise in a way that is significant enough to justify using it as opposed to the more commonly understood one.

This is something I've run into quite a bit with the compiler, and, notably, I have almost always ended up having to use the more precise term due to certain edge cases being included that couldn't be classified by the more narrow term.

With that in mind, I just had a look through the compiler code to see exactly what an 'external library` is.

Specifically, I wanted to see if an 'external library' could reasonably have some outside case that isn't well represented by the term 'node modules'. From what I've observed, I'm of the opinion that it does not in this case. (Feel free to correct me if I'm wrong!)

Throughout the tests, I noted that all examples of 'external libraries' have a node_modules path.

Then I also found this line:
https://github.com/microsoft/TypeScript/blob/8cbc57695461b86252a6085ae4d31ad64993e43a/src/compiler/moduleNameResolver.ts#L334

This shows that isExternalLibrary is determined by pathContainsNodeModules, and throughout the rest of the resolver code, it seems as though all uses apply to a node modules path.

Considering all that, I think that overwriteNodeModules might be a bit more clear to the lay-person, as it seems to be idiomatically more broadly understood. What do you think?

[danielpza]

@nonara alright, looks good to me then, thanks for the research. And you @icaro-capobianco for putting up the PR :D

[nonara]

@icaro-capobianco Looks good, thanks for the work! I'll be adding a few other features here in the next week or so, so I will add some tests and roll this into that release.

[nonara]

Looks like I don't have access to make changes on this. I'm going to go ahead and merge and apply styling in a separate PR.

Thanks @icaro-capobianco for the PR! Good work.

[nonara]

Quick update on this - in further digging into this, the proposed solution had some issues.

Namely, the option implies that it allows overriding a node_modules import, but in practice, the solution only overrides node_modules if the new path is a URL. It would not work, for example, if you were attempting to override with another path. This would lead to issues being filed by people expecting it to function as the name implies.

To compensate, we could write logic to allow full override, but this would mean diverging from or replicating TypeScript's logic for module resolution, which is something that we do not want to do.

However, in order to help you with your situation, I've introduced the @transform-path tag. This should allow you to specify a custom output path for any statement.

In your case, you can use:

// @transform-path https://cdnjs.cloudflare.com/ajax/libs/preact/10.5.7/preact.module.min.js
import { render, h } from 'preact';

Hope this helps, and thanks again for the contribution.