cevek / ttypescript

Over TypeScript tool to use custom transformers in the tsconfig.json
1.53k stars 56 forks source link

Expose modified TypeScript object #18

Closed ddsol closed 6 years ago

ddsol commented 6 years ago

Some plugins may want to patch the TypeScript object in order to provide, for instance, the ability to allow decorators on any node. Because the TypeScript module that will be acquired with import * from 'ts' will be a separate module instance, this will no longer allow modification of the currently active ts library.

This is overcome by exporting the patched ts module instance from where it can be imported by plugin modules that support this.

ddsol commented 6 years ago

Not sure why tests are failing. Looks like my code doesn't change much in regards to functionality.

cevek commented 6 years ago

Because the TypeScript module that will be acquired with import * from 'ts' will be a separate module instance, this will no longer allow modification of the currently active ts library

Node js shares all required modules. So if you require module in one place then patch it, other modules that require this module also will use this patched version.

For your example if you require var ts = require('typescript') in your transformer - it should be already patched

ddsol commented 6 years ago

This is not what happens in this case. I think this is because the module gets loaded into a VM. Also, this is probably the reason the patched module gets passed from tsc.ts to patchCreateProgram. If it could just require the module, then it wouldn't be necessary to pass the patched version through.

I'm also thinking this might cause issue with things like instanceof because an instance of a class from the patched version is not an instance of the mirror class from the unpatched version. I don't know if it will matter in the case of TypeScript. A quick search of the TypeScript codebase tells me they don't make much use of instanceof (except in tests). Mosts tests are performed using the kind property.

It might also be possible to inject the patched version of ts into the normal module cache, leading others to require the patched version.

In any case, I have a transformer that needs to patch ts and it does not work if I don't use the TTypeScript-patched version of the TyperScript module.

ddsol commented 6 years ago

I think the reason there are 2 modules is not because of the VM-loading. I think the reason is because the patched version module isn't being loaded by Module, but it's just executed in a VM. As such it doesn't get added to the module cache, so that of anyone else requires TS, it loads a new copy.

ddsol commented 6 years ago

Hmm. I just added the require cache override. That worked great. Then I removed the require cache override, and it still worked great.

This whole PR seems to be moot. I have no idea how it's working, but it's working. I also don't understand why it wasn't working before. I still have a some questions on how the module makes it into the module cache, or which code puts it there, but it's more curiosity than need, so I'm just going to close this PR.

cevek commented 6 years ago

Make sure that you don't using "esModuleInterop": true in your tsconfig.json cause it replaces original exported object with a copy.

If it could just require the module, then it wouldn't be necessary to pass the patched version through.

patchCreateProgram just a function that will patch a ts and it can be from tsc.ts, typescript.ts, typescriptServices.ts, tsserver.ts etc...

All original ts files are patched just by requiring them, but tsc.ts if different - there is evaled original tsc.ts. So, if you run ttsc in your terminal you cannot patch it in your transformer by require("typescript"). But it will work for ts-node or webpack ts-loader modules.

I can provide ts module as third argument to the transformer if you want to patch it. But your patches will be applied only after ts.transpileModule() or ts.createProgram() calls in the typescript.

Could you explain what are you want to patch in ts, and maybe it will be helpful for everyone

ddsol commented 6 years ago

I need to patch nodeCanBeDecorated to allow decorators on interfaces.

I just did some more tests and as it turns out, there are 2 versions of the module, as well as 2 copies of each class defined in ts.

I previously had this PR so that it would provide the ts as the third argument, which I backtracked on when the tests were massively failing.

Just now I changed tsc.js (ttsc) to:

const content = `const ts = require('typescript');\n` + fs
    .readFileSync(tscFileName, 'utf8')
    .replace('ts.executeCommandLine(ts.sys.args);', 'module.exports = ts;');

This seems to work just fine. Not sure what the countLines is supposed to do, but everything compiles and there's only one version of the module.

Is there a reason not to use this version?

ddsol commented 6 years ago

Sure. Testing...

ddsol commented 6 years ago

It's not working, because I'm on Windows and the slashes break the ${typescriptFilename} part of the template string.

ddsol commented 6 years ago

It works when I change it to ${typescriptFilename.replace(/\\/g, '/')}

cevek commented 6 years ago

I've rewrote tsc.ts Try again please

ddsol commented 6 years ago

That one works. The changes got me thinking: Would it be possible to require ts, patch it, then simply require the original tsc, including the executeCommandLine? No eval would be needed, and the tsc could remain as it is.

cevek commented 6 years ago

It is not possible, because original tsc contains whole typescript in itself.

ddsol commented 6 years ago

Would it be possible to get this released? Otherwise I'd have to have to build a workaround to make this work when there's this perfectly good package already here.

cevek commented 6 years ago

@ddsol I've made some refactoring, and now ttypescript do not patch typescript directly - it makes copy of them. Also I expose patched ts as third argument to transformer.

export default function myTransformerPlugin(program: TS.Program, opts: MyPluginOptions, { ts }: {ts: typeof TS}) {