libjass icon indicating copy to clipboard operation
libjass copied to clipboard

Update Typescript version

Open qgustavor opened this issue 5 years ago • 6 comments

The current Typescript version is returning this warning: DeprecationWarning: Buffer() is deprecated due to security and usability issues.

Trying to update to a newer version breaks the code:

~/build/typescript/compiler.js:239
exports.oldGetLeadingCommentRangesOfNodeFromText = ts.getLeadingCommentRangesOfNodeFromText.bind(ts);
                                                                                            ^

TypeError: Cannot read property 'bind' of undefined
    at Object.<anonymous> (~/build/typescript/compiler.js:239:93)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Module.require (internal/modules/cjs/loader.js:636:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (~/build/typescript/index.js:21:18)
    at Module._compile (internal/modules/cjs/loader.js:688:30)

Why Typescript is being extended by compiler.ts? Is possible to rewrite build.js without depending on those changes?

qgustavor avatar Jun 04 '19 15:06 qgustavor

The build step uses the typescript compiler API rather than tsc directly to add JSDoc annotations to the output (@constructor, @enum, @memberof, etc) and for generating the docs, both of which require type information.

It also modifies the compiler output using UJS with some custom steps to strip multiple definitions of the compiler helpers like __extends and to minify better by renaming private members (UJS does not rename them by default because JS doesn't have a concept of private members).

But both of these custom steps are only for quality-of-life improvements. Just running tsc should produce a usable file that has the same API as the one built with the build script, and just minifying it with any minifier at default settings will produce a usable minified file.

Arnavion avatar Jun 04 '19 16:06 Arnavion

The build step uses the typescript compiler API [...]

So I need to update the build step to use the new compiler API in order to make it work with the new Typescript version.

Just running tsc should produce a usable file that has the same API as the one built with the build script [...]

I don't know a lot about Typescript, but I tried running npm install --save-dev typescript@latest; npx tsc --outfile lib\libjass.js src\index.ts and it returned those errors and no file. Am I missing something?

Edit: I tried npx tsc -p src\tsconfig.json and it's now returning results. As I'm compiling it for Node I removed dom lib and I need to remove parts of code that depend on it.

Edit 2: I got it to compile without warnings. Instead of upgrading I just removed those quality-of-life improvements and features that I'm not planning to use (like rendering and web worker support). You can close this issue if you prefer.

qgustavor avatar Jun 04 '19 17:06 qgustavor

So I need to update the build step to use the new compiler API in order to make it work with the new Typescript version.

To clarify what I said, the current script hooks into the compiler internals because "add JSDoc comments to the compiler output" is not something the compiler API supports. So it's almost certain it won't work without modification if you update the compiler, which is why you got the error that you got in the OP.

I tried running npm install --save-dev typescript@latest; npx tsc --outfile lib\libjass.js src\index.ts and it returned those errors and no file. Am I missing something?

As you found, you need to give it the path of the project file (tsconfig.json), not the index.ts

As I'm compiling it for Node I removed dom lib and I need to remove parts of code that depend on it.

Did you do this because you got compiler errors with the new TS's dom.d.ts ? Or just to slim down the output?

Edit 2: I got it to compile without warnings. Instead of upgrading

Right, I wasn't suggesting that you actually try to update the build script code to work with newer TS. That's why I said to just run tsc directly.


I didn't really have a plan to update the TS version because the current one is good enough for a project that isn't being developed any more, ie it works and produces a usable output. Newer TS obviously has more typing features but they aren't particularly necessary for a library that only gets shipped as JS.

But if the current TS version stops working at some point because node.js itself breaks it (say by removing the deprecated Buffer constructor), I suppose there is value in getting rid of the custom build stuff.

Arnavion avatar Jun 04 '19 18:06 Arnavion

Did you do this because you got compiler errors with the new TS's dom.d.ts ? Or just to slim down the output?

I'm making a fork focused on subtitle parsing, modification and serialization. I used to work with ass-parser, but it's a bit feature lacking compared with libjass. I'm sure I would mess things if I tried to implement tag parsing myself - because I already messed it - so I decided to make a fork.

I wasn't suggesting that you actually try to update

If I updated it I could send a pull request fixing this issue. I needed to remove docs, if I fix the build script I can get the docs working again and I can help fixing this issue.

qgustavor avatar Jun 04 '19 19:06 qgustavor

In terms of priorities, the work is:

  1. Update TS to current version, even if at the cost of not having custom build steps for docs or synthesized JSDoc comments.

  2. Be able to build docs. IIRC this relies entirely on public TS compiler API, so it might still need to be updated but it won't be using any hacks.

  3. Be able to synthesize JSDoc comments. As I said before, currently this uses hacks to be able to access the compiler internals. But it seems TS had added a compiler API for it - https://github.com/microsoft/TypeScript/blame/master/lib/typescript.d.ts#L4110 - so hopefully the hacks aren't needed any more.


If you're up to doing (2) and even (3), that would be amazing! But you said you aren't familiar with TS, and I imagine even less familiar with the TS compiler API. So even if you send a PR for just (1) that'll be great.

Arnavion avatar Jun 04 '19 23:06 Arnavion

I'm trying to fix everything: started with a fresh clone, updated Typescript to latest version and tried fixing errors or commenting parts of the code until it compiles. By now it's compiling, but Field ... has no @type annotation., There are @param annotations ... and Unbound type parameter ... are silenced (probably because TS API changed) and docs aren't building. If possible I will send a pull request soon.

Edit: I will not send a pull request because my code isn't working. Those are the changes I did at the moment: https://github.com/qgustavor/libjass/commit/218a0952efd87129fde3471d1573f737c2965c38

I thought that if I just looked change logs and commits on Typescript repository and used those to update the code to the newer Typescript API it would solve the issue, but seems it's more complicated than I expected. It's using not only the public Typescript API but also the private API too. Maybe is better rewriting the JSDoc adding part of code... I will pause working on it, I need to rewrite parts of my code that were using ass-parser to use libjass now.

qgustavor avatar Jun 05 '19 22:06 qgustavor