Closed SalvatorePreviti closed 5 years ago
I really don't understand what is problem. Nodejs module system executes modules once and then put the module.exports of this module into a cache. So ttypescript parses and executes original typescript only once.
There is problem in the patchCreateProgram
that it uses original required typescript's methods like parseJsonConfigFileContent
, readConfigFile
etc. So I fixed it in the latest commit
Yes I fixed in this one too, let me checkout the latest and will try to explain the rationale behind this.
Since we are using a custom js loader (before it was eval
, now is vm,runInThiscontext
) we are skipping the cache mechanism of require
.
What happens with an example:
In the code on master when we run tsc
on a project with custom transformer X
this may happen:
node_modules/typescript/lib/typescript.js
as string (no cache involved)node_modules/typescript/lib/tsc.js
as string, replacing ts with the loaded typescript (no cache involved)createProgram
that loads custom transformer X
.X
has on top require('typescript')
.node_modules/typescript/lib/typescript.js
is not in the cache, a new instance of node_modules/typescript/lib/typescript.js
is parsed, executed and loaded.All is good, everything works but we are loading typescript.js
twice, is not a poblem but it can be faster if we put the parsed typescript.js
in the require.cache
with a lazy load approach.
This PR skips reading and parsing of typescript.js if already loaded through ttypescript loadTypeScript
function. The module typescript/lib/typescript.js
will not be parsed and compiled twice by V8, but will still loaded from the compiled javascript without the custom createProgram
patched, as it was behaving before.
Yeah, I know about this problem of copy version of typescript in the transformer. Firstly ttypescript just replaced original typescript createProgram like
var ts = require('typescript');
ts.createProgram = function() {
... patched version
}
But later we decided to have real copy of typescript see #19
I think we should refactor current version to ttypescript/clone
and just patch original ts in the default
Yes indeed, I understood you wanted a real copy of it. This PR would keep a real copy of typescript. It just skips the parsing of it, the behaviour is the same but just the performances are better because it don't parse typescript.js twice, also if it instantiate it twice.
I don't understand how do you want keep two version of typescript(patched and original) with only one parsing?
When loading a js module there are 4 steps involved: read file, compile file, execute file (this generate a function), execute the function.
We can skip the first 3 steps executing only the function in the end, since we parsed and executed typescript.js
So, to simplify, vm.runInThisContext returns a function. If we execute the returned function twice is like we load a module twice, but of course the parsing and compilation involved in vm.runInThisContext (that is expensive for big files) will be executed only once.
Said this, I like the idea of ttypescript/clone. Ttypescript/clone may still benefit from this small optimisation.
We can skip the first 3 steps executing only the function in the end, since we parsed and executed typescript.js
But this is problem because execution result will be patched in future, but original typescript should be pristine
So, to simplify, vm.runInThisContext returns a function. If we execute the returned function twice is like we load a module twice
I don't understand how we can execute runInThisContext twice?
We don't execute runInThisContext twice, but it was potentially executed twice because require('typescript')
does.
In this implementation, typescript.js will be pristine. We are registering with require.cache['...node_modules/typescript/lib/typescript.js']
a custom Lazy module that will just skip the 3 steps we already performed before patching
So, we, as NodeJS does, call runInThisContext() with a wrapped function. We wrap the typescript.js in a function. runInThisContext() returns a function that when executed instantiates a new pristine typescript. We can call this returned function multiple times to create as many pristine typescript instances as we want. We can then use one of the typescripts returned by this function to pass to patchCreateProgram, but we can also register one of these pristine istances in the nodeJS cache, with a lazy loader module.exports getter, so nodeJS will not call runInThisContext twice if we already did on typescript.js
I get it, but I think it is too complicated
Can we simplify code in loadTypescript
to?:
if (!require.cache[typescriptFilename]) {
const originalTsModule = new Module(typescriptFilename, module);
Object.defineProperty(originalTsModule, 'exports', {
get() {
if (!this._exports) {
const m = { exports: {} };
typescriptFactory.call(m, m.exports, require, m, typescriptFilename, dirname(typescriptFilename));
this._exports = m.exports;
}
return this._exports;
},
});
require.cache[typescriptFilename] = originalTsModule;
}
Let me try to think a bit on how to simplify. That is the most performant way I can think of, but maybe I can write a functional "lazyload" decorator or function
Can we make createPristineTypeScriptLazyModule
more simple?
function createPristineTypeScriptLazyModule(filename: string, factory: TypeScriptModuleFactory) {
const m = new Module(filename, module);
m.filename = filename;
m.loaded = true;
m.paths = module.paths.slice();
let exports: ts | undefined;
Object.defineProperty(m, 'exports', {
get() {
if (!exports) {
const m = { exports: {} };
factory.call(m, m.exports, require, m, filename, dirname(filename));
exports = m.exports;
}
return exports;
},
enumerable: true,
configurable: true
});
return m;
}
or more readable
function createPristineTypeScriptLazyModule(filename: string, factory: TypeScriptModuleFactory) {
return new class extends Module {
filename = filename;
loaded = true;
paths = module.paths.slice();
private _exports?: ts;
constructor() {
super(filename, module);
}
get exports() {
if (!this._exports) {
const m = { exports: {} };
factory.call(m, m.exports, require, m, filename, dirname(filename));
this._exports = m.exports;
}
return this._exports;
}
}
Good job! Thank you! :)
Thank you!
The custom loader parses and patches the various .js files of typescript compiler. Typescript compiler libraries are heavy, big files with a lot of functions to parse an compile. We can potentially improve loading times by avoiding v8 to parse and compile again
require('typescript.js')
. The idea is to modify the custom loader to register a custom lazy load modules in the require.cache that will not parse again the js code. This should result in better loading times.