Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment: Remove all internal typescript code, manage bundle manually #4750

Closed
wants to merge 29 commits into from

Conversation

ry
Copy link
Member

@ry ry commented Apr 14, 2020

design doc: https://docs.google.com/document/d/1_WvwHl7BXUPmoiSeD8G83JmS8ypsTPqed4Btkqkn_-4/edit?usp=sharing

incremental build time:
1m 50s -> 1m 0s

release exe size (mac):
42mb -> 41mb

@ry ry changed the title Remove all internal typescript code Experiment: Remove all internal typescript code, manage bundle manually Apr 14, 2020
@lucacasonato
Copy link
Member

I understand where this is coming from, nonetheless I don't like it. This would make #2180 nearly impossible because we have just one file, so we can not split ops into crates that contain both Rust and JS. A lot of future value for deno_core would be lost.

Also we are removing Typescript - that means we don't have any type checking on internal Deno code at all 😢. The amount bugs that typescript can catch is crazy and I would argue that no code reviewer can do the same level of static analysis. There will be bugs that TS would have caught.


We have two independent TS hosts: one for the internal Deno code in //deno_typescript/compiler_main.js and another other for external user code in //cli/js/compiler/. These two hosts have similar goals - they seek to modify TSC to operate on Deno-style imports (e.g. with file extensions). The //deno_typescript is used at build-time while the //cli/js/compiler/ is used at runtime.

What if we used a previously compiled version of deno to transpile the internal typescript code to js? It would remove the need for the special deno_typescript for internal code. This would mean that deno will require itself to be built which might pose a challenge for adding new target platform in the future (should still be doable though). On the other hand it would let us benefit from all of the work going into //cli/js/compiler/. With some work //cli/js/compiler/ should be capable of emitting d.ts files next to just JS, which would probably allow us to generate runtime type definitions automatically making a desync between definitions and implementation nearly impossible and thus resolving point 2 and 4 of the problem list.

@nayeemrmn
Copy link
Collaborator

Can't we keep types and just check and strip them independently of the d.ts file? If deno_typescript/compiler_main.ts is a problem is it possible to replace it with proper self-hosting as Luca suggested or even tsc/Node? I think most problems can still be solved without losing all of that type information... think of all the doc that wasn't written, brevity when choosing variable names, less robust code patterns employed, etc. because we had types to supplement.

As far as being easy to understand for new contributors goes, we should go with the slow approach and not work on a SystemJS bundle. Unless there a slow way of stripping SystemJS afterwards? I'd hate to be stuck with it. Also, I remember this coming for something else long ago, why do we default to cli/rt.js instead of cli/runtime.js?

@ry
Copy link
Member Author

ry commented Apr 15, 2020

Tests are passing in debug mode, but hitting a segfault in release mode which might be a memory leak that is being exposed by the new structure.

@ry
Copy link
Member Author

ry commented Apr 16, 2020

we will be pursing the removal of typescript from the internal code, but this PR isn't ready yet. We need to do a few more experiments. See the updated discussion in https://docs.google.com/document/d/1_WvwHl7BXUPmoiSeD8G83JmS8ypsTPqed4Btkqkn_-4/edit?usp=sharing

@ry ry closed this Apr 16, 2020
@nayeemrmn nayeemrmn mentioned this pull request May 25, 2020
6 tasks
@robpalme
Copy link
Contributor

robpalme commented Jun 9, 2020

Has anyone run the TypeScript compiler diagnostics to get a breakdown on the super slow incremental builds? We did this recently and found that a small change made incremental TypeScript builds go 6x faster.

@wjohnsto
Copy link

wjohnsto commented Jun 22, 2020

I ran into some similar TS problems to what Deno is experiencing when building PlatypusTS, and what I ended up doing was creating ts-bundle, which essentially builds the TS based on a references.d.ts into one file, combining the code based on namespaces. In the end you get single TS file to run through TSC. This leads to a more predictable JS output as well as a more reliable declaration file. I would never really consider this in an application/server development project, but it does work for a framework that wants to produce usable .d.ts files and minimize the additional/weird code that TS creates when you have "conflicts."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants