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

refactor: reorganize TS compiler #5603

Merged
merged 5 commits into from
May 20, 2020

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented May 18, 2020

Yet another refactor of TS compiler:

  • made bundle() a standalone function instead of method of TsCompiler
  • TS compiler doesn't depend on TextEncoder/TextDecoder
  • removed dead code from TS compiler
  • fixed typos

Fixes #5507

Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far... let me know if you want/need anything

@bartlomieju
Copy link
Member Author

bartlomieju commented May 18, 2020

LGTM so far... let me know if you want/need anything

@kitsonk I want TS worker to not use any console logs - so I want to move processConfigureResponse to Rust; can you look into that?

EDIT: commonPath and normalizeUrl also look shady... If we can remove them/move logic into Rust

@kitsonk
Copy link
Contributor

kitsonk commented May 18, 2020

processConfigureResponse

That is going to be hard-ish because it relies on ts APIs to ensure that stuff can be parsed, and it supports JSONC, and serde specifically refuses to support JSONC. I will have to look at swc and see if they support parsing of tsconfig.json files easily enough. Will look into it.

EDIT: commonPath and normalizeUrl also look shady... If we can remove them/move logic into Rust

They aren't shady! :-) I am not too sure we want to move them, because they are tightly coupled to how TypeScript generates the bundle (as in you have to "guess" at what the module specifiers are going to be because of this: microsoft/TypeScript#35052). They should go when we move the bundle generation out to Rust (as in all we do is take the single outfile and do the munging in Rust) which doesn't seem like we are ready to do that quite yet.

@bartlomieju
Copy link
Member Author

processConfigureResponse

That is going to be hard-ish because it relies on ts APIs to ensure that stuff can be parsed, and it supports JSONC, and serde specifically refuses to support JSONC. I will have to look at swc and see if they support parsing of tsconfig.json files easily enough. Will look into it.

dprint supports parsing of JSONC dprint/dprint@ce9b5a6 - maybe @dsherret can expose parser?

EDIT: commonPath and normalizeUrl also look shady... If we can remove them/move logic into Rust

They aren't shady! :-) I am not too sure we want to move them, because they are tightly coupled to how TypeScript generates the bundle (as in you have to "guess" at what the module specifiers are going to be because of this: microsoft/TypeScript#35052).

I see, thanks for explanation I couldn't really grok what they are for.

They should go when we move the bundle generation out to Rust (as in all we do is take the single outfile and do the munging in Rust) which doesn't seem like we are ready to do that quite yet.

How would it take? I already do some munching on source maps generated during runtime code compilation so that shouldn't be a big deal.

@dsherret
Copy link
Member

dsherret commented May 18, 2020

@bartlomieju the JSONC parser is here: https://github.com/dprint/jsonc-parser

I just recently added the parse_to_value function that you could use here.

https://docs.rs/jsonc-parser/0.7.0/jsonc_parser/fn.parse_to_value.html

@bartlomieju bartlomieju added the cli related to cli/ dir label May 18, 2020
@kitsonk
Copy link
Contributor

kitsonk commented May 18, 2020

I broke out the other two things into #5606 and #5607 so I could explain a bit more what is going on or needs to go on in my opinion as well as make sure it doesn't get lost. small bites

@bartlomieju bartlomieju changed the title [WIP] another ts compiler refactor refactor: reorganize TS compiler May 20, 2020
@bartlomieju bartlomieju requested a review from ry May 20, 2020 12:02
@bartlomieju
Copy link
Member Author

@ry let's land this PR for 1.0.1

@@ -673,7 +632,7 @@ function buildSourceFileCache(
}
}

interface EmmitedSource {
interface EmittedSource {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are all private interfaces not visible to end users

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice clean up

@bartlomieju bartlomieju merged commit 8799855 into denoland:master May 20, 2020
@bartlomieju bartlomieju deleted the ts_compiler_refactor branch May 20, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript compile error during bundle gives unhelpful assertion error
4 participants